Amun contest - Ruhum's results

Handle

Ruhum

Vulnerability details

Impact

According to the ERC20 standard, the approve() function returns a boolean value indicating whether the call was successful or not, see https://eips.ethereum.org/EIPS/eip-20#methods

The SingleJoin/SingleExit contracts ignore that return value when approving tokens for the uniswap router. It might lead to a failed transaction because the contract proceeded with the swap although the approve() call failed.

Proof of Concept

relevant line: https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExit.sol#L44

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExitV2.sol#L55

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleTokenJoin.sol#L43

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleTokenJoinV2.sol#L53

Tools Used

Slither

Recommended Mitigation Steps

Use SafeERC20.safeApprove() which reverts when the call fails or returns false.

Handle

Ruhum

Vulnerability details

Impact

The EthSingleTokenJoin and EthSingleTokenJoinV2 contracts both rely on the INTERMEDIATE_TOKEN to be WETH. But, both contracts can be deployed with a token other than WETH which would make them unusable. In that case, the contract would have to be redeployed. There's no way of adjusting that state variable after deployment.

To mitigate that, it shouldn't be possible to deploy those two contracts with a token other than WETH through address checks in the constructor.

Proof of Concept

Relevant constructors: https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/EthSingleTokenJoin.sol#L10

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

Tools Used

none

Recommended Mitigation Steps

Verify that the passed _INTERMEDIATE_TOKEN address is actually the WETH contract.

Handle

Ruhum

Vulnerability details

Impact

The SingleJoin & SingleExit contracts use transfer() to send ether to the caller. If the caller is a smart contract the call might fail reverting the transaction if:

• caller contract isn't able to receive ether (missing receive() or payable fallback() function)
• caller contract uses more than the provided 2300 gas when receiving ether

Since gas costs might increase in the future, using transfer() might cause these contracts to potentially break since transfer() would always fail.

Here's a blogpost about using transfer(): https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

I'd rate the issue as medium since a part of the protocol would potentially be unusable.

Proof of Concept

Usage of transfer():

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/EthSingleTokenJoin.sol#L37

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

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExit.sol#L93

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExitV2.sol#L121

Tools Used

none

Recommended Mitigation Steps

Instead, the contracts should use:

(bool success, ) = payable(msg.sender).call{value: intermediateTokenBalance}("");
require(success);

Note that this approach introduces a risk of reentrancy. But, that shouldn't be a problem for these specific contracts. Nevertheless, it might be worth adding reentrancy guards in case the contracts ever get extended.