Amun contest - GiveMeTestEther's results

Handle

GiveMeTestEther

Vulnerability details

Impact

Use of transfer() might render ETH impossible to withdraw becuase after istanbul hardfork , there is increases in the gas cost of the SLOAD operation and therefore breaks some existing smart contracts.Those contracts will break because their fallback functions used to consume less than 2300 gas, and they’ll now consume more, since 2300 the amount of gas a contract’s fallback function receives if it’s called via Solidity’s transfer() or send() methods. Any smart contract that uses transfer() or send() is taking a hard dependency on gas costs by forwarding a fixed amount of gas: 2300.

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Proof of Concept

2021-12-amun\contracts\basket\contracts\singleJoinExit\EthSingleTokenJoin.sol: msg.sender.transfer(remainingIntermediateBalance);

2021-12-amun\contracts\basket\contracts\singleJoinExit\EthSingleTokenJoinV2.sol: msg.sender.transfer(remainingIntermediateBalance);

2021-12-amun\contracts\basket\contracts\singleJoinExit\SingleNativeTokenExit.sol: msg.sender.transfer(intermediateTokenBalance);

2021-12-amun\contracts\basket\contracts\singleJoinExit\SingleNativeTokenExitV2.sol: msg.sender.transfer(intermediateTokenBalance);.

Tools Used

Manual Analysis

Recommended Mitigation Steps

-use call() to send ETH

Handle

GiveMeTestEther

Vulnerability details

Impact

The function "_maxApprove()" calls IERC20.approve() that is defined to return a boolean. However, there are legitimate non-standards-compliant tokens like USDT that don’t return a return value on approve(). All IERC20.approve() calls to such tokens will revert because of the missing return value.

Proof of Concept

https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExit.sol#L39 https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExitV2.sol#L50 https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/singleJoinExit/SingleTokenJoin.sol#L38 https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/singleJoinExit/SingleTokenJoinV2.sol#L48

Tools Used

Manual Analysis

Recommended Mitigation Steps

• use safeApprove() everywhere instead of approve()

Handle

GiveMeTestEther

Vulnerability details

Impact

Some tokens need to first set the the approve amount to 0 to set it to a non-zero amount (USDT: https://etherscan.io/address/0xdac17f958d2ee523a2206206994597c13d831ec7#code). This makes the _maxApprove() and the approve.selector calls incompatible to such tokens.

Proof of Concept

approve.selector: https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/callManagers/RebalanceManagerV3.sol#L75 https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/callManagers/RebalanceManagerV2.sol#L70 https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/callManagers/RebalanceManager.sol#L131 https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/callManagers/RebalanceManager.sol#L84

.approve(): https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExit.sol#L39 https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExitV2.sol#L50 https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/singleJoinExit/SingleTokenJoin.sol#L38 https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/singleJoinExit/SingleTokenJoinV2.sol#L48

Tools Used

Recommended Mitigation Steps

• set the approval amount to 0 before setting it to a non-zero amount

Handle

GiveMeTestEther

Vulnerability details

Impact

Every ETH that wasn't received by "receive() external payable" from "INTERMEDIATE_TOKEN" by calling "withdraw()" cannot be withdrawn. Because in the contract we only transfer INTERMEDIATE_TOKEN.balanceOf(address(this)) and not the ETH balance of the contract, that also could be sent accidentally.

Proof of Concept

2021-12-amun\contracts\basket\contracts\singleJoinExit\EthSingleTokenJoin.sol: receive() external payable {} 2021-12-amun\contracts\basket\contracts\singleJoinExit\EthSingleTokenJoinV2.sol: receive() external payable {} 2021-12-amun\contracts\basket\contracts\singleJoinExit\SingleNativeTokenExit.sol: receive() external payable {} 2021-12-amun\contracts\basket\contracts\singleJoinExit\SingleNativeTokenExitV2.sol: receive() external payable {}

Tools Used

Manual Analysis

Recommended Mitigation Steps

• only allow address(INTERMEDIATE_TOKEN) to call the receive() with a require statement

Handle

GiveMeTestEther

Vulnerability details

Don't need to use safe subtraction in "uint256 timePassed = block.timestamp.sub(lastFeeClaimed);" because block.timestamp is always bigger or equal to lastFeeClaimed.

Impact

Save gas by not doing a safe subtraction.

Proof of Concept

https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/facets/Basket/BasketFacet.sol#L257

Tools Used

Manual Analysis

Recommended Mitigation Steps

• write L257 as: uint256 timePassed = block.timestamp - lastFeeClaimed;

Handle

GiveMeTestEther

Vulnerability details

"bs.exitFee" can only be <= MAX_EXIT_FEE (1017 = 10 %) So the feeAmount can be at max 10% of _amount. uint256 feeAmount = _amount.mul(bs.exitFee).div(1018); Therefore the subtarction _amount.sub(feeAmount) can't underflow and can write this as "_amount - feeAmount" (save gas by not doin underflow check)

uint256 tokenAmount = tokenBalance.mul(_amount.sub(feeAmount)).div(totalSupply);

Impact

No need to do safe subtraction and can save some ags.

Proof of Concept

https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/facets/Basket/BasketFacet.sol#L207

https://github.com/code-423n4/2021-12-amun/blob/98f6e2ff91f5fcebc0489f5871183566feaec307/contracts/basket/contracts/facets/Basket/BasketFacet.sol#L385

Tools Used

Manual Analysis

Recommended Mitigation Steps

• write L207/388 as: tokenBalance.mul(_amount - feeAmount).div(totalSupply);