Skip to content
This repository has been archived by the owner on Jun 30, 2024. It is now read-only.

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

Open
sherlock-admin2 opened this issue Dec 26, 2023 · 3 comments
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A valid High severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin2
Copy link

sherlock-admin2 commented Dec 26, 2023

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

@sherlock-admin sherlock-admin changed the title Massive Grey Barracuda - Incorrect deviation check Massive Grey Barracuda - Incorrect ProtocolOwnedLiquidityOhm calculation due to inclusion of other user's reserves Dec 28, 2023
@github-actions github-actions bot added the Excluded Excluded by the judge without consulting the protocol or the senior label Dec 30, 2023
@nevillehuang nevillehuang reopened this Dec 30, 2023
@nevillehuang nevillehuang added High A valid High severity issue and removed Excluded Excluded by the judge without consulting the protocol or the senior labels Dec 30, 2023
@0xJem 0xJem added Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed labels Dec 30, 2023
@0xJem
Copy link

0xJem commented Dec 30, 2023

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

@nevillehuang nevillehuang added the Has Duplicates A valid issue with 1+ other issues describing the same vulnerability label Jan 8, 2024
@sherlock-admin2 sherlock-admin2 changed the title Massive Grey Barracuda - Incorrect ProtocolOwnedLiquidityOhm calculation due to inclusion of other user's reserves hash - Incorrect ProtocolOwnedLiquidityOhm calculation due to inclusion of other user's reserves Jan 8, 2024
@sherlock-admin2 sherlock-admin2 added the Reward A payout will be made for this issue label Jan 8, 2024
@0xrusowsky
Copy link

@IAm0x52
Copy link
Collaborator

IAm0x52 commented Jan 12, 2024

Fix looks good. OnlyOwner modifier has been added to deposits

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A valid High severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

5 participants