Amun contest
Findings & Analysis Report

2023-03-07

Table of contents

Overview

Please note: Code4rena is an organization that puts learning at the forefront of everything we do. Our rules and processes continue to develop over time, and older reports may reflect previous iterations of these rules and processes. For a more current representation of Code4rena’s severity standardization rules and comprehensive judging criteria, we recommend browsing the reports from C4’s most recent contests.

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 code contest is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the code contest outlined in this document, C4 conducted an analysis of Amun smart contract system written in Solidity. The code contest took place between December 13—December 19 2021.

Wardens

30 Wardens contributed reports to the Amun contest:

  1. WatchPug (jtp and ming)
  2. Czar102
  3. certora
  4. csanuragjain
  5. cmichel
  6. kenzo
  7. robee
  8. harleythedog
  9. JMukesh
  10. gpersoon
  11. hyh
  12. defsec
  13. pauliax
  14. pedroais
  15. jayjonah8
  16. pmerkleplant
  17. p4st13r4 (0xb4bb4 and 0x69e8)
  18. gzeon
  19. GiveMeTestEther
  20. Ruhum
  21. Jujic
  22. itsmeSTYJ
  23. ye0lde
  24. 0x1f8b
  25. sirhashalot
  26. Dravee
  27. 0x0x0x
  28. saian
  29. shenwilly
  30. hubble (ksk2345 and shri4net)

This contest was judged by 0xleastwood.

Final report assembled by burgertime and CloudEllie.

Summary

The C4 analysis yielded an aggregated total of 35 unique vulnerabilities and 129 total findings. All of the issues presented here are linked back to their original finding.

Of these vulnerabilities, 2 received a risk rating in the category of HIGH severity, 10 received a risk rating in the category of MEDIUM severity, and 23 received a risk rating in the category of LOW severity.

C4 analysis also identified 44 non-critical recommendations and 50 gas optimizations.

Scope

The code under review can be found within the C4 Amun contest repository, and is composed of 45 smart contracts written in the Solidity programming language and includes 585 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities according to a methodology based on OWASP standards.

Vulnerabilities are divided into three primary risk categories: high, medium, and low.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

  • Malicious Input Handling
  • Escalation of privileges
  • Arithmetic
  • Gas use

Further information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website.

High Risk Findings (2)

[H-01] Unused ERC20 tokens are not refunded, and can be stolen by attacker

Submitted by WatchPug

Under certain circumstances, e.g. annualizedFee being minted to feeBeneficiary between the time user sent the transaction and the transaction being packed into the block and causing amounts of underlying tokens for each basketToken to decrease. It’s possible or even most certainly that there will be some leftover basket underlying tokens, as BasketFacet.sol#joinPool() will only transfer required amounts of basket tokens from Join contracts.

However, in the current implementation, only the leftover inputToken is returned.

As a result, the leftover underlying tokens won’t be returned to the user, which constitutes users’ fund loss.

SingleTokenJoinV2.sol L57-L78

function joinTokenSingle(JoinTokenStructV2 calldata _joinTokenStruct)
    external
{
    // ######## INIT TOKEN #########
    IERC20 inputToken = IERC20(_joinTokenStruct.inputToken);

    inputToken.safeTransferFrom(
        msg.sender,
        address(this),
        _joinTokenStruct.inputAmount
    );

    _joinTokenSingle(_joinTokenStruct);

    // ######## SEND TOKEN #########
    uint256 remainingIntermediateBalance = inputToken.balanceOf(
        address(this)
    );
    if (remainingIntermediateBalance > 0) {
        inputToken.safeTransfer(msg.sender, remainingIntermediateBalance);
    }
}

BasketFacet.sol L143-L168

function joinPool(uint256 _amount, uint16 _referral)
    external
    override
    noReentry
{
    require(!this.getLock(), "POOL_LOCKED");
    chargeOutstandingAnnualizedFee();
    LibBasketStorage.BasketStorage storage bs =
        LibBasketStorage.basketStorage();
    uint256 totalSupply = LibERC20Storage.erc20Storage().totalSupply;
    require(
        totalSupply.add(_amount) <= this.getCap(),
        "MAX_POOL_CAP_REACHED"
    );

    uint256 feeAmount = _amount.mul(bs.entryFee).div(10**18);

    for (uint256 i; i < bs.tokens.length; i++) {
        IERC20 token = bs.tokens[i];
        uint256 tokenAmount =
            balance(address(token)).mul(_amount.add(feeAmount)).div(
                totalSupply
            );
        require(tokenAmount != 0, "AMOUNT_TOO_SMALL");
        token.safeTransferFrom(msg.sender, address(this), tokenAmount);
    }
    ...

Furthermore, the leftover tokens in the SingleTokenJoinV2 contract can be stolen by calling joinTokenSingle() with fake outputBasket contract and swap.exchange contract.

Consider:

  1. Calling IBasketFacet.calcTokensForAmount() first and only swap for exactly the desired amounts of tokens (like SingleTokenJoin.sol);
  2. Or, refund leftover tokens.

loki-sama (Amun) acknowledged

[H-02] It might not be possible to withdraw tokens from the basket

Submitted by Czar102, also found by csanuragjain

Impact

When enough basket token owners exit, it will be impossible to exit pool with the last MIN_AMOUNT tokens because of this check. This will result in locking some tokens forever.

Consider resigning from this check or performing it only for the owner balance, who would need to have at least MIN_AMOUNT tokens.

loki-sama (Amun) disagreed with severity

0xleastwood (Judge) commented:

Nice find! I think this is valid:)

Medium Risk Findings (10)

[M-01] Function joinTokenSingle in SingleTokenJoin.sol and SingleTokenJoinV2.sol can be made to fail

Submitted by pmerkleplant, also found by certora, hyh, p4st13r4, pauliax, robee, and WatchPug

Impact

There’s a griefing attack vulnerability in the function joinTokenSingle in SingleTokenJoin.sol as well as SingleTokenJoinV2.sol which makes any user transaction fail with “FAILEDOUTPUTAMOUNT”.

Proof of Concept

The JoinTokenStruct argument for joinTokenSingle includes a field outputAmount to indicate the amount of tokens the user should receive after joining a basket (see line 135 and 130).

However, this amount is compared to the contract’s balance of the token and reverts if the amount is unequal.

If an attacker sends some amount of a basket’s token to the contract, every call to this function will fail as long as the output token equals the attacker’s token send.

Refactor the require statement to expect at least the outputAmount of tokens, i.e. require(outputAmount >= _joinTokenStruct.outputAmount).

loki-sama (Amun) confirmed

[M-02] Unchecked return value from low-level call()

Submitted by JMukesh, also found by certora

Impact

The return value of the low-level call is not checked, so if the call fails, the Ether will be locked in the contract. If the low level is used to prevent blocking operations, consider logging failed calls.

Proof of Concept

https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/singleJoinExit/EthSingleTokenJoinV2.sol#L26

add condition to check return value

loki-sama (Amun) confirmed and disagreed with severity

0xleastwood (Judge) commented:

Nice find! I think this could be marked as medium as it leaks value from the protocol but it doesn’t result in assets being lost directly. It requires _INTERMEDIATE_TOKEN to point to a contract which fails upon wrapping the ETH amount.

So considering that _INTERMEDIATE_TOKEN must be improperly set, I will mark this as medium.

[M-03] It is possible to “uninitialize” ERC20Facet contract

Submitted by Czar102

Impact

The initialization status is defined by the name and symbol. It is possible it set them back to an empty string, uninitializing the contract and letting the initialize(..) function be called again. This way, the owner may, for example, hide minting additional tokens. Or, after accidentally setting name and symbol to empty strings, anyone can take control over the contract and mint any number of tokens.

In general, it shouldn’t be possible to initialize more than once.

Consider adding empty string checks in setName(...) and setSymbol(...) functions.

loki-sama (Amun) confirmed

[M-04] Annualized fee APY dependence on the frequency of executing a function

Submitted by Czar102

Impact

The APY of the annualized fee is dependent on the frequency of the execution of the BasketFacet::chargeOutstandingAnnualizedFee(). If it is called more frequently, the compounding is more frequent and the APY is higher. For less used baskets, the APY might be lower, because the compounding will happen at lower rate.

Consider calculating the fee as the compounding was continous or with a constant compounding period.

loki-sama (Amun) acknowledged

0xleastwood (Judge) commented:

Nice find!

[M-05] totalSupply may exceed LibBasketStorage.basketStorage().maxCap

Submitted by Czar102, also found by gpersoon, gzeon, kenzo, and WatchPug

Impact

Total supply of the token may exceed the maxCap introduced. This can happen when a user wants to join the pool. The check in BasketFacet::joinPool(...) includes only the base amount, without fee. Thus, if fee is on and someone will want to create as many tokens as possible, the totalSupply + _amount will be set to maxCap. The call will succeed, but new tokens were also minted as the fee for bs.feeBeneficiary if bs.entryFee and bs.entryFeeBeneficiaryShare are nonzero. Thus, the number of tokens may exceed maxCap.

Consider calculating feeAmount and feeBeneficiaryShare before the require(...) statement and check totalSupply.add(_amount).add(feeBanficiaryShare) <= this.getCap().

loki-sama (Amun) acknowledged

[M-06] block.timestamp or deadline

Submitted by gpersoon, also found by kenzo and robee

Impact

Some functions, like rebalance() in RebalanceManagerV3 use _deadline as a time limit for swapExactTokensForTokens() Other functions, like _joinTokenSingle() of SingleTokenJoinV2.sol and _exit() of SingleNativeTokenExitV2() use block.timestamp, although a deadline field is present in the struct.

Possibly the deadline fields should have been used.

Proof of Concept

RebalanceManagerV3.sol L158-L203

function rebalance(UnderlyingTrade[] calldata _swapsV2, uint256 _deadline)  external override onlyRebalanceManager {
...
        for (uint256 i; i < _swapsV2.length; i++) {
  ...
            for (uint256 j; j < trade.swaps.length; j++) {
                ..
                _swapUniswapV2(swap.exchange,input,0, swap.path,address(basket), _deadline );

RebalanceManagerV3.sol L63-L104

function _swapUniswapV2(...) {
        basket.singleCall(
            exchange,
            abi.encodeWithSelector(  IUniswapV2Router02(exchange).swapExactTokensForTokens.selector,  quantity,   minReturn,  path, recipient, deadline  ),
            0
        );

SingleTokenJoinV2.sol L80-L112

struct JoinTokenStructV2 {
     ...
        uint256 deadline;
     ...
    }
function _joinTokenSingle(JoinTokenStructV2 calldata _joinTokenStruct)  internal {
     ...
           for (uint256 j; j < trade.swaps.length; j++) {
                IPangolinRouter(swap.exchange).swapExactTokensForTokens( amountIn,  0, swap.path, address(this),  block.timestamp );
            }
        }

SingleNativeTokenExitV2.sol L59-L88

 struct ExitTokenStructV2 {
        ...
        uint256 deadline;
       ...
    }
function _exit(ExitTokenStructV2 calldata _exitTokenStruct) internal {
     ...
        for (uint256 i; i < _exitTokenStruct.trades.length; i++) {
           ...
            for (uint256 j; j < trade.swaps.length; j++) {
                ...
                IPangolinRouter(swap.exchange).swapExactTokensForTokens( IERC20(swap.path[0]).balanceOf(address(this)), 0, swap.path, address(this), block.timestamp );
            }
        }

Check whether the deadline fields should have been used. If so replace block.timestamp with the appropriate deadline

loki-sama (Amun) confirmed

[M-07] ERC20 return values not checked

Submitted by cmichel, also found by defsec, JMukesh, p4st13r4, and WatchPug

The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead.

See:

  • SingleNativeTokenExitV2.exit’s outputToken.transfer(msg.sender, outputTokenBalance);
  • PieFactoryContract.bakePie’s pie.transfer(msg.sender, _initialSupply);

Impact

Tokens that don’t actually perform the transfer and return false are still counted as a correct transfer and the tokens remain in the SingleNativeTokenExitV2 contract and could potentially be stolen by someone else.

We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.

0xleastwood (Judge) commented:

Nice find! I think this is valid considering the extent basket tokens are used. It is more than likely that non-standard tokens will be utilised.

[M-08] SingleNativeTokenExitV2 assumes first exchange holds the outputToken

Submitted by kenzo, also found by cmichel and hyh

SingleNativeTokenExitV2 allows the user to exit and execute trades via multiple exchanges. When finishing the trades and sending a single output token back to the user, the contract takes that token from the last swap in the first exchange’s trades. There is nothing in the struct that signifies this will be the output token, and this also impairs the exit functionality.

Impact

Let’s say a basket only holds token TOKE, and user would like to exit to DAI. But there’s no exchange with good liquidity for TOKE -> DAI. So the user crafts a trade to exchange TOKE for WOKE in exchange A, and then exchange WOKE for DAI in exchange B, to finally receive back DAI. The contract will not let him do so, as the output token is taken to be the output token of the first exchange - WOKE in our example.

Proof of Concept

In exit, the output token is taken to be the last token exchanged in the first exchange: (Code ref)

address[] calldata path = _exitTokenStruct
            .trades[0]
            .swaps[_exitTokenStruct.trades[0].swaps.length - 1]
            .path;
        IERC20 outputToken = IERC20(path[path.length - 1]); //this could be not the target token

This manifests the issue I detailed above.

Have the outputToken be a parameter supplied in ExitTokenStructV2.

loki-sama (Amun) acknowledged

[M-09] Failed transfer with low level call could be overlooked

Submitted by harleythedog

Impact

The CallFacet.sol contract has the function _call :

function  _call(
	address  _target,
	bytes  memory  _calldata,
	uint256  _value
) internal {
	require(address(this).balance >= _value, "ETH_BALANCE_TOO_LOW");
	(bool success, ) = _target.call{value: _value}(_calldata);
	require(success, "CALL_FAILED");
	emit  Call(msg.sender, _target, _calldata, _value);
}

This function is utilized in a lot of different places. According to the Solidity docs, “The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed”.

As a result, it is possible that this call will not work but _call will not notice anything went wrong. It could be possible that a user is interacting with an exchange or token that has been deleted, but _call will not notice that something has gone wrong and as a result, ether can become stuck in the contract. For this reason, it would be better to also check for the contract’s existence prior to executing _target.call.

For reference, see a similar high severity reported in a Uniswap audit here (report # 9): https://github.com/Uniswap/v3-core/blob/main/audits/tob/audit.pdf

Proof of Concept

See _call here: CallFacet.sol L108.

To ensure tokens don’t get stuck in edge case where user is interacting with a deleted contract, make sure to check that contract actually exists before calling it.

loki-sama (Amun) confirmed

[M-10] fees calculations are not accurate

Submitted by certora

after that fee is calculated, it is minted to the feeBeneficiary. simply minting the exact amount results lower fee than it should be.

Impact

feeBeneficiary will get less fees than it should.

Proof of Concept

let’s assume that the basket assets are worth 1M dollars, and totalSupply = 1M. the result of calcOutStandingAnnualizedFee is 100,00 so the feeBeneficiary should get 100,00 dollars. however, when minting 100,00 the totalSupply will increase to 1,100,000 so they will own 100000/1100000 * (1M dollars) = 90909.09 dollars instead of 100k

loki-sama (Amun) acknowledged:

This is mitigated by the feeBeneficiary diluting his own shares if he gets fees on his fees.

0xleastwood (Judge) asked:

I’m not exactly sure if I understand what the warden is stating here. Could you confirm @loki-sama ?

loki-sama (Amun) confirmed:

Ok, I myself misunderstood. He is correct that we don’t get the full value. When we take a fee of 10% like from his example. What we do is mint 10% of the basket to ourselves. That 10% after minting is not holding 10% of the underling.

Low Risk Findings (23)

Non-Critical Findings (44)

Gas Optimizations (50)

Disclosures

C4 is an open organization governed by participants in the community.

C4 Contests incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Contest submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.