Vulnerability regarding the `tradeAll` parameter of swap()

[code423n4]

Handle

daejunpark

Vulnerability details

Impact

The tradeAll parameter of the swap() function in the modules can be exploited to drain any existing locked tokens of the Executioner contract. This behavior effectively nullifies the access control for rescueTokens(), except for the native token case.

Scenario

1. Suppose the Executioner contract happens to hold a number of X tokens for some reasons (e.g., users accidentally “donating” funds to it, or leftovers of previous trades having accumulated).
2. Then, adversaries can call executeTrades() where the first element of trades is given the calldata for a swap() call with tradeAll == true and tokenIn == X.
3. Then, the first swap() call will swap all the X tokens held by the Executioner contract, and return the swapped output to the adversaries.

Recommendation

For the short term, clearly document this behavior so that users take extra caution to not leave any leftovers nor accidently “donating” funds to Executioner.

For the long term, improve the contract logic so that each trades[i] execution can be given the result of the previous trades[j] for j < i, and the tradeAll logic can utilize the historical information.

[tommyz7]

As stated in contest's readme this is expected behavior and works as designed. It's invalid.

[alcueca]

Dispute accepted.