search

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

0 stars 0 forks source link

Regular Trades Can Drain Executioner.sol Balance #21

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

zer0dot

Vulnerability details

Impact

This vulnerability allows any trade to effectively drain the balance, as long as it is not the chain's native asset (which is not used directly in the modules) , from the Executioner.sol contract. This is technically not critical because funds are not supposed to remain in the Executioner.sol contract.

However, the existence of the rescueTokens() function may be redundant, and its execution may be frontrun if an attacker is aware of this vulnerablity.

Proof of Concept

Here is how an attack would unfold:

Aassume DAI balance of Executioner = 1, the attack is successful if the 1 DAI is drained, used values are just for the sake of the example:

  1. Craft a trade with fromToken = DAI, fromAmount = 0.001 and toToken = USDC
  2. Encode calldata so that the trade amount is 0.001 + 1 = 1.001 DAI in the trade.encodedCallData variable, or tradeAll = true.
  3. Execute the trade: a. The initial balance of the toToken (USDC) will equal 0 b. _transferFromOrWrap will transfer 0.001 DAI to the Executioner c. executioner.executeTrades(trades) will execute the trade with encoded calldata, trading 1.001 DAI for, for example, 1.001 USDC d. finalOutputAmount will equal 1.001 - 0 = 1.001 USDC e. executioner.sendFunds(toToken, _msgSender(), finalOutputAmount) will transfer 1.001 USDC to the original message sender, effectively draining the executioner's balance.

Recommended Mitigation Steps

The ideal mitigation would be to not allow user-crafted calldata to be executed via delegateCall on the modules from the Executioner contract. This would ensure that the trade parameters passed to the original executeTrades() function in the Slingshot.sol contract are the ones that are used for the actual swap.

tommyz7 commented 2 years ago

In context of 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."

finding is invalid/out of scope.

alcueca commented 2 years ago

Dispute accepted.