obront - Non terminal tokens received via Balancer batchSwap are not accounted for

[sherlock-admin]

obront

high

Non terminal tokens received via Balancer batchSwap are not accounted for

Summary

Balancer's batchSwap() function is able to return multiple tokens along the execution path, but BalancerController.sol only accounts for receipt of the final token in the swap. The result is that other tokens received from a user's trade will not be registered with the protocol or included in their account.

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).

For users who would like all their funds to move from step to step (swapping all of their input token from their first step, for the maximum amount of the output token from the final step), Balancer provides the helpful option to set amount = 0 for all steps after the first, and it will automatically use the maximum amount.

Alternatively, users are able to provide their own amounts in each step. Any amount returned from a step that is not used for the next step is sent to the user.

Here is a simple example:

• Step 1: Trade 400 BAL for max WETH (returns ~1.7)
• Step 2: Trade 1 WETH for max USDC (returns ~1575)
• Result: 400 BAL is inputted, and both 0.7 WETH and 1575 USDC are returned

Here is a gist with a standalone Foundry test that can be run to show this behavior.

As the canBatchSwap() function is currently implemented, only the final asset (in this case, USDC) would be returned in the tokensIn[] array. While the user's WETH would be transferred to their account, it would not be registered with the protocol to count towards their account balance or interact with.

Impact

If a user uses Balancer's batchSwap() functionality with returned intermediate tokens that are not in the account's asset list already, the user may get liquidated sooner than expected as RiskEngine.sol:_getBalance() only counts in the assets in the assets list.

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-L228

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. Didn't follow recommendation above, but blocked all trades with intermediate amounts > 0, so problem is solved.