Vagner - BalancerWeightedAuraVault.sol wrongly assumes that all of the weighted pools uses totalSupply

[sherlock-admin]

Vagner

high

BalancerWeightedAuraVault.sol wrongly assumes that all of the weighted pools uses totalSupply

Summary

BalancerWeightedAuraVault.sol 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 docs here
https://docs.balancer.fi/concepts/advanced/valuing-bpt/valuing-bpt.html#getting-bpt-supply .
The docs specifies the fact that totalSupply is only used for older stable and weighted pools, and should not be used without checking it first, since the newer pools have pre-minted BPT and getActualSupply should be used in that case. Most of the time, the assumption would be that only the new composable stable pools uses the getActualSupply, but that is not the case, since even the newer weighted pools have and uses the getActualSupply. To give you few examples of newer weighted pools that uses getActualSupply
https://etherscan.io/address/0x9f9d900462492d4c21e9523ca95a7cd86142f298
https://etherscan.io/address/0x3ff3a210e57cfe679d9ad1e9ba6453a716c56a2e
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 _mintVaultShares and _calculateLPTokenValue would both be inaccurate, which would hurt the protocol and the users.

Impact

Impact is a high one since the calculation of shares and the value of the LP tokens is very important for the protocol, and any miscalculations could hurt the protocol and the users a lot.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/BalancerWeightedAuraVault.sol#L1-L94

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.

[jeffywu]

Thanks, good finding.

[jeffywu]

notional-finance/leveraged-vaults#69

[gstoyanovbg]

As I read the report, I left with the impression that the getActualSupply function should be used because there is some amount of pre-minted BPT tokens. However, this is not true in the case of weighted pools. In those cases, we don't have pre-minted tokens, and consequently, there is no virtual supply. This is evident from the links to the contracts provided in the description. This is the code of the getActualSupply function.

    /**
     * @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);
    }

From the comment, it is apparent that this function should be used because the pool owes debt to the Protocol in the form of unminted BPT, which should be taken into consideration. Despite the incorrect reason, I agree that getActualSupply should be used instead of totalSupply. I'm not entirely sure about the impact, and I won't comment on it. I'm writing this comment because the report will be read by people after the contest, and they would receive incorrect information.

[xiaoming9090]

Reviewed PR 69 and observed that the wrong interface is used. The IWeightedPool interface should be used instead of IComposablePool interface since this is a weighted pool vault.

[jeffywu]

@xiaoming9090 made a fix here: notional-finance/leveraged-vaults#82

Although the method signature is the same for both contracts so this doesn't make any real functional change. Agree that it is more clear this way.

[xiaoming9090]

Fixed in PR 82

[jeffywu]

@gstoyanovbg Yes your comment is correct. We followed up with the Balancer team directly on this. Newer Weighted Pools have this method exposed when fees are paid in BPT inflation. We would prefer to use this method over totalSupply to ensure that we have the correct denominator for the total BPT. I believe the two numbers are "trued up" when new LP tokens are minted so they should not diverge excessively over time.