obront - User can trade away an asset using Balancer batchSwap without removing it from account

[sherlock-admin]

obront

medium

User can trade away an asset using Balancer batchSwap without removing it from account

Summary

Balancer's batchSwap() function takes in an amount of tokens to trade at each step of the swap. This amount can actually exceed the amount returned from the previous step. Because BalancerController.sol uses only the first token in the chain as tokensOut, a user can abuse this functionality to empty their account of a token while bypassing the expected removeAsset calls.

Vulnerability Detail

Balancer's batchSwap() functionality takes in an array of BatchSwapSteps, each of which dictates a pool to use for the swap, assets to swap to and from, an amount, and some additional user data (which is currently unused by Balancer).

The expectation is that users will either leave amount = 0 (which will autofill the amount returned from the last step) or will set amount manually to some smaller value.

However, it is also possible to input an amount that is greater than the value returned by the previous step, if you own those tokens and you have already approved the Balancer contract to trade them.

Using this technique, a user could send their tokens to Balancer without having them flagged by Sentiment as tokensOut. In an extreme case, this would allow them to get rid of all their tokens, without their account being updated to reflect this.

Here is an example:

• A user has 400 BAL and 100 WETH
• They call batchSwap() with the path BAL => WETH => USDC
• For the first step, they set amount = 400
• For the second step, they set amount = [amount returned from step 1] + 100
• The result is that they will have traded all of their BAL and WETH into USDC, but only BAL will appear in tokensOut

Here is a gist with a proof of concept showing a user decreasing their WETH balance without WETH appearing in tokensOut.

Impact

Users have the ability to use the batchSwap() functionality to throw off the accounting on their account, removing a token but keeping the token in their assets array and keeping hasAsset[token] = true.

Code Snippet

https://github.com/sherlock-audit/2022-11-sentiment/blob/main/controller-merged/src/balancer/BalancerController.sol#L193-L197

https://github.com/sherlock-audit/2022-11-sentiment/blob/main/controller-merged/src/balancer/BalancerController.sol#L219-L222

Tool used

Manual Review, Foundry

Recommendation

Balancer's batchSwap() function returns int256[] memory assetDeltas, which provide details on the final amounts of each asset that are inputted and outputted from the function.

Rather than try to manually calculate these values, the best option is likely the following:

• Use an in-line assembly block to STATICCALL Balancer's canBatchSwap() to return the asset deltas
• Any positive number in assetDeltas means the token belongs in tokensOut
• Any negative number in assetDeltas means the token belongs in tokensIn

[r0ohafza]

PR: sentimentxyz/controller#49

[zobront]

Confirmed fix. Didn't follow recommendation above, but blocked all trades with intermediate amounts > 0, so problem is solved.