0xMR0 - `BalancerPoolTokenPrice.getWeightedPoolTokenPrice()` wrongly assumes that all of the weighted pools uses `totalSupply()`

[sherlock-admin2]

0xMR0

high

BalancerPoolTokenPrice.getWeightedPoolTokenPrice() wrongly assumes that all of the weighted pools uses totalSupply()

Summary

BalancerPoolTokenPrice.getWeightedPoolTokenPrice() which is used specifically for weighted balancer pools wrongly uses totalSupply all the time to get the total supply of the pool, but this would not be true for newer weighted pools.

Vulnerability Detail

Balancer pools have different methods to get their total supply of minted LP tokens, which is also specified in the balancer documentation.

Reference link: https://docs.balancer.fi/concepts/advanced/valuing-bpt/valuing-bpt.html#getting-bpt-supply

The documentation states that totalSupply is only used for older stable and weighted pools, and should not be used without checking it first.

totalSupply In general, totalSupply only makes sense to call for older "legacy" pools. The original Weighted and Stable Pools do not have pre-minted BPT, so they follow the typical convention of using totalSupply to account for issued pool shares

since the newer pools have pre-minted BPT and getActualSupply should be used in that case.

getActualSupply This is the most common/current function to call. getActualSupply is used by the most recent versions of Weighted and Stable Pools. It accounts for pre-minted BPT as well as due protocol fees.

Lets check the getActualSupply() function code,

    /**
     * @notice Returns the effective BPT supply.
     *
     * @dev This would be the same as `totalSupply` however the Pool owes debt to the Protocol in the form of unminted
     * BPT, which will be minted immediately before the next join or exit. We need to take these into account since,
     * even if they don't yet exist, they will effectively be included in any Pool operation that involves BPT.
     *
     * In the vast majority of cases, this function should be used instead of `totalSupply()`.
     *
     * **IMPORTANT NOTE**: calling this function within a Vault context (i.e. in the middle of a join or an exit) is
     * potentially unsafe, since the returned value is manipulable. It is up to the caller to ensure safety.
     *
     * This is because this function calculates the invariant, which requires the state of the pool to be in sync
     * with the state of the Vault. That condition may not be true in the middle of a join or an exit.
     *
     * To call this function safely, attempt to trigger the reentrancy guard in the Vault by calling a non-reentrant
     * function before calling `getActualSupply`. That will make the transaction revert in an unsafe context.
     * (See `whenNotInVaultContext` in `WeightedPool`).
     *
     * See https://forum.balancer.fi/t/reentrancy-vulnerability-scope-expanded/4345 for reference.
     */
    function getActualSupply() external view returns (uint256) {
        uint256 supply = totalSupply();

        (uint256 protocolFeesToBeMinted, ) = _getPreJoinExitProtocolFees(
            getInvariant(),
            _getNormalizedWeights(),
            supply
        );

        return supply.add(protocolFeesToBeMinted);
    }

As seen in getActualSupply() function comment, it is apparent that getActualSupply() function should be used because the pool owes debt to the Protocol in the form of unminted BPT, which should be taken into consideration.

Newer Weighted Pools have getActualSupply() method when fees are paid in BPT inflation.

Most of the time, the assumption would be that only the new composable stable and weighted pools uses the getActualSupply, but that is not the case, since even the newer weighted pools have and uses the getActualSupply.

Let's have a look at below examples of newer weighted pools that uses getActualSupply

1) https://etherscan.io/address/0x9f9d900462492d4c21e9523ca95a7cd86142f298 2) https://etherscan.io/address/0x3ff3a210e57cfe679d9ad1e9ba6453a716c56a2e 3) https://etherscan.io/address/0xcf7b51ce5755513d4be016b0e28d6edeffa1d52a

the last one also being on Aura finance. Because of that, the interaction with newer weighted pools and also the future weighted pools would not be accurate since the wrong total supply is used and calculations like getWeightedPoolTokenPrice() will return the incorrect/inaccurate price, which would hurt the protocol and the users.

Reference: This issue is very much referenced from this issue which was found in Notional audit at Sherlock by Vagner.

Impact

Impact is a high one since the calculation of Weighted Pool Token Price is very important for the protocol, and any miscalculations could hurt the protocol and the users a lot. Most recent pools used will return wrong pool token price.

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/PRICE/submodules/feeds/BalancerPoolTokenPrice.sol#L402

Tool used

Manual Review

Recommendation

Since the protocol would want to interact with multiple weighted pools, try to check first if the weighted pool you are interacting with is a newer one and uses the getActualSupply or if it is an older one and uses totalSupply, in that way the protocol could interact with multiple pools in the long run.

Duplicate of #155