search

code-423n4 / 2021-10-slingshot-findings

0 stars 0 forks source link

Leftover balance in the Executioner contract can be drained #20

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

gzeon

Vulnerability details

Impact

Leftover balance in the Executioner contract can be drained by swapping the target asset(native/erc20) into another asset.

Slingshot.executeTrades() allow user to execute trade using modules as long as the module is registered in the ModuleRegistry. The Slingshot contract check for initial balance of toToken (Slingshot.sol:L81) in the Executioner before the swap to make sure it does not include existing balance. However, an attacker can simply swap the token into some other token to bypass this check.

Proof of Concept

1) Assuming we have 100USDC leftover in the Executioner 2) Attacker call Slingshot.executeTrades() with the following parameters address fromToken: WETH address toToken: USDT uint256 fromAmount: 0 TradeFormat[] calldata trades: [( address moduleAddress = UniswapModule bytes encodedCalldata = UniswapModule.swap( uint256 amount = 0 address[] memory path = [USDC, USDT] bool tradeAll = True ) )] uint256 finalAmountMin: 0 3) The attacker will be able to extract the 100USDC as USDT

Recommended Mitigation Steps

Under normal circumstance there should be no balance leftover in the contract, except for situation like user mistakenly send token to the contract, aridrop, or when the supplied trade did not use the entire amount. As I noticed the team is concerned with these situation (therefore Slingshot.sol:L81), I think this is an actual issue need to be fixed although this require some specific user behavior. To fix this would require the swap modules to check for balance of each asset used at each step and/or return the amountOut value for checking.

Alternatively, if the team think the additional complexity and gas usage would not be worth it we can drop the balance check at L81 and make this a gas optimization instead.

tommyz7 commented 2 years ago

As stated in contest readme: "rescueTokens and rescueTokensFromExecutioner can be gamed however it is not a concern. They are in place "just in case" and should not be used in the first place."

The same design has been used in the previous version of contracts and so far we have not had a single occurrence of "leftover". It's invalid in my opinion.

alcueca commented 2 years ago

Even if the sponsor didn't specifically state that leftover tokens are not a concern, the issue is not valid.

If there would be a fund loss from this vulnerability, it is not a loss felt by anyone. Leftover tokens are considered lost even before an attacker gets to them, and there are no plans to rescue them.