sherlock-audit / 2023-11-olympus-judging

9 stars 8 forks source link

tvdung94 - balancerPool.totalSupply() might not give correct results for newer weighted pools #122

Closed sherlock-admin2 closed 9 months ago

sherlock-admin2 commented 9 months ago

tvdung94

medium

balancerPool.totalSupply() might not give correct results for newer weighted pools

Summary

balancerPool.totalSupply() might not give correct results for newer weighted pools

Vulnerability Detail

As stated by official balancer docs, it is recommended to use getActualSupply() instead for newer weighted pools, since they have fees paid in BPT.

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

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

Impact

Core functions depending on this totalSupply() method might return incorrect results.

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider use getActualSupply instead

Duplicate of #155