cuthalion0x - `BalancerPairOracle` can be manipulated using read-only reentrancy

[sherlock-admin]

cuthalion0x

high

BalancerPairOracle can be manipulated using read-only reentrancy

Summary

BalancerPairOracle.getPrice makes an external call to BalancerVault.getPoolTokens without checking the Balancer Vault's reentrancy guard. As a result, the oracle can be trivially manipulated to liquidate user positions prematurely.

Vulnerability Detail

In February, the Balancer team disclosed a read-only reentrancy vulnerability in the Balancer Vault. The detailed disclosure can be found here. In short, all Balancer pools are susceptible to manipulation of their external queries, and all integrations must now take an extra step of precaution when consuming data. Via reentrancy, an attacker can force token balances and BPT supply to be out of sync, creating very inaccurate BPT prices.

Some protocols, such as Sentiment, remained unaware of this issue for a few months and were later hacked as a result.

BalancerPairOracle.getPrice makes a price calculation of the form f(balances) / pool.totalSupply(), so it is clearly vulnerable to synchronization issues between the two data points. A rough outline of the attack might look like this:

AttackerContract.flashLoan() ->
    // Borrow lots of tokens and trigger a callback.
    SomeProtocol.flashLoan() ->
        AttackerContract.exploit()

AttackerContract.exploit() ->
    // Join a Balancer Pool using the borrowed tokens and send some ETH along with the call.
    BalancerVault.joinPool() ->
        // The Vault will return the excess ETH to the sender, which will reenter this contract.
        // At this point in the execution, the BPT supply has been updated but the token balances have not.
        AttackerContract.receive()

AttackerContract.receive() ->
    // Liquidate a position using the same Balancer Pool as collateral.
    BlueBerryBank.liquidate() ->
        // Call to the oracle to check the price.
        BalancerPairOracle.getPrice() ->
            // Query the token balances. At this point in the execution, these have not been updated (see above).
            // So, the balances are still the same as before the start of the large pool join.
            BalancerVaul.getPoolTokens()

            // Query the BPT supply. At this point in the execution, the supply has already been updated (see above).
            // So, it includes the latest large pool join, and as such the BPT supply has grown by a large amount.
            BalancerPool.getTotalSupply()

            // Now the price is computed using both balances and supply, and the result is much smaller than it should be.
            price = f(balances) / pool.totalSupply()

        // The position is liquidated under false pretenses.

Impact

Users choosing Balancer pool positions (such as Aura vaults) as collateral can be prematurely liquidated due to unreliable price data.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/oracle/BalancerPairOracle.sol#L70-L92

Tool used

Manual Review

Recommendation

The Balancer team recommends utilizing their official library to safeguard queries such as Vault.getPoolTokens. However, the library makes a state-modifying call to the Balancer Vault, so it is not suitable for view functions such as BalancerPairOracle.getPrice. There are then two options:

1. Invoke the library somewhere else. Perhaps insert a hook into critical system functions like BlueBerryBank.liquidate.
2. Adapt a slightly different read-only solution that checks the Balancer Vault's reentrancy guard without actually entering.

[Gornutz]

https://github.com/Blueberryfi/blueberry-core/commit/e5348bc649822f6e96c9c7be25d2b6a06d5a6318#diff-d8a6f8d601164fa5805ceb20da1f406bef6792d6f33fa002c258d1f63ccb25b3

[IAm0x52]

Use same PR as #25 from update contest 2. The issue of read-only reentrancy has been addressed but will be waiting for update to PR with different library before signing off.