sherlock-audit / 2023-11-olympus-judging

9 stars 7 forks source link

hash - Incorrect ProtocolOwnedLiquidityOhm calculation due to inclusion of other user's reserves #172

Open sherlock-admin2 opened 6 months ago

sherlock-admin2 commented 6 months ago

hash

high

Incorrect ProtocolOwnedLiquidityOhm calculation due to inclusion of other user's reserves

Summary

ProtocolOwnedLiquidityOhm for Bunni can include the liquidity deposited by other users which is not protocol owned

Vulnerability Detail

The protocol owned liquidity in Bunni is calculated as the sum of reserves of all the BunniTokens

    function getProtocolOwnedLiquidityOhm() external view override returns (uint256) {

        uint256 len = bunniTokens.length;
        uint256 total;
        for (uint256 i; i < len; ) {
            TokenData storage tokenData = bunniTokens[i];
            BunniLens lens = tokenData.lens;
            BunniKey memory key = _getBunniKey(tokenData.token);

        .........

            total += _getOhmReserves(key, lens);
            unchecked {
                ++i;
            }
        }

        return total;
    }

The deposit function of Bunni allows any user to add liquidity to a token. Hence the returned reserve will contain amounts other than the reserves that actually belong to the protocol


    // @audit callable by any user
    function deposit(
        DepositParams calldata params
    )
        external
        payable
        virtual
        override
        checkDeadline(params.deadline)
        returns (uint256 shares, uint128 addedLiquidity, uint256 amount0, uint256 amount1)
    {
    }

Impact

Incorrect assumption of the protocol owned liquidity and hence the supply. An attacker can inflate the liquidity reserves The wider system relies on the supply calculation to be correct in order to perform actions of economical impact

https://discord.com/channels/812037309376495636/1184355501258047488/1184397904551628831
it will be determined to get backing
so it will have an economical impact, as we could be exchanging ohm for treasury assets at a wrong price

Code Snippet

POL liquidity is calculated as the sum of bunni token reserves https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/SPPLY/submodules/BunniSupply.sol#L171-L191

BunniHub allows any user to deposit https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/external/bunni/BunniHub.sol#L71-L106

Tool used

Manual Review

Recommendation

Guard the deposit function in BunniHub or compute the liquidity using shares belonging to the protocol

0xJem commented 6 months ago

This is a good catch, and the high level is justified

0xrusowsky commented 6 months ago

https://github.com/OlympusDAO/bophades/pull/260

IAm0x52 commented 5 months ago

Fix looks good. OnlyOwner modifier has been added to deposits