Amun contest - hyh's results

Handle

hyh

Vulnerability details

Impact

Griefing attack is possible for all basket joining contracts, making them fail all the time, disabling the join functionality for them altogether.

Basket join will be permanently failing if join contract balance grew by any amount, which is what any existing basket token holder can achieve. An attacker can send tiny amount of basket tokens to join contracts and joining the basket will be failing all the time. This will be the permanent state as there is no way to empty the balance of join contract.

The reason is strict equality as success condition of _joinTokenSingle functions of SingleTokenJoin and SingleTokenJoinV2 contracts. As EthSingleTokenJoin and EthSingleTokenJoinV2 use the affected _joinTokenSingle functions, all joining mechanics will be disabled. The mentioned contracts then to be fixed and redeployed.

Proof of Concept

The condition used for successful obtaining of required set of tokens is strict equality: https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleTokenJoin.sol#L134

It can be easily manipulated to failing by sending any amount of basket to the join contract.

This is possible as basket ERC20 do not censor transfers: https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/facets/ERC20/ERC20Facet.sol#L138

Recommended Mitigation Steps

a. Either perform balance check twice, before and after all the operations, and require the difference to be equal to _joinTokenStruct.outputAmount

b. Or, for gas saving and clarity, replace the condition with inequality, so that a user joining will get at least required amount of basket tokens. Now:

require(outputAmount == _joinTokenStruct.outputAmount, "FAILED_OUTPUT_AMOUNT");

To be:

require(outputAmount >= _joinTokenStruct.outputAmount, "FAILED_OUTPUT_AMOUNT");

Handle

hyh

Vulnerability details

Impact

Exit swaps in each trade are restricted to (basket_token_i, ..., WETH) sequences as the output token is read from the first trade. This way any other correct trade structures will fail the function because of the output token amount check.

Proof of Concept

SingleNativeTokenExitV2.exit assumes that all trades will have the same output token and reads the output token from the first trade: https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExitV2.sol#L93

When trades are more complicated than that due to liquidity or suitability considerations, for example, user wants to swap all the basket tokens to WETH, then WETH to USDC and withdraw it, or when basket is (t1, t2, WBTC) user may want to swap t1 to WBTC, t2 to WETH, then WBTC to WETH, and withdraw it, current logic fails and function will be reverted because of the final output token amount check: https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExitV2.sol#L100

Recommended Mitigation Steps

Fix the index to locate the last token in the swap sequence:

Now:

address[] calldata path = _exitTokenStruct
		.trades[0]
		.swaps[_exitTokenStruct.trades[0].swaps.length - 1]
		.path;

To be:

uint tradesLastIndex_ = _exitTokenStruct.trades.length - 1;
address[] calldata path = _exitTokenStruct
		.trades[tradesLastIndex_]
		.swaps[_exitTokenStruct.trades[tradesLastIndex_].swaps.length - 1]
		.path;

Also, the comment looks like it is temporary, can it be clarified? https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExitV2.sol#L96