sherlock-audit / 2023-11-olympus-judging

9 stars 7 forks source link

tvdung94 - BunniPrice::_getBunniReserves() does not add uncollected fee into total value calculation #137

Closed sherlock-admin closed 6 months ago

sherlock-admin commented 6 months ago

tvdung94

high

BunniPrice::_getBunniReserves() does not add uncollected fee into total value calculation

Summary

BunniPrice::_getBunniReserves() does not add uncollected fee into total value calculation.

Vulnerability Detail

A bunni token is an erc20 token representing a uniswapv3 position with specific range. When the price enters this range, the position will get reward (uncollected fee). The fee is separated from the position, so it needs to be manually added to total value.

Impact

Incorrect reserve amount, since fee is not accounted into total value.

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider add fee into calculation

    function _getBunniReserves(
        BunniToken token_,
        BunniLens lens_,
        uint8 outputDecimals_
    ) internal view returns (address token0, uint256 reserve0, address token1, uint256 reserve1) {
        BunniKey memory key = _getBunniKey(token_);
        (uint112 reserve0_, uint112 reserve1_) = lens_.getReserves(key);
    >>>    (uint256 fee0, uint256 fee1) = lens_.getUncollectedFees(key);

        // Get the token addresses
        token0 = key.pool.token0();
        token1 = key.pool.token1();
        uint8 token0Decimals = ERC20(token0).decimals();
        uint8 token1Decimals = ERC20(token1).decimals();
   >>>     reserve0 = uint256(reserve0_ + fee0).mulDiv(10 ** outputDecimals_, 10 ** token0Decimals);
   >>>     reserve1 = uint256(reserve1_ +fee1).mulDiv(10 ** outputDecimals_, 10 ** token1Decimals);
    }

Duplicate of #37

sherlock-admin2 commented 6 months ago

1 comment(s) were left on this issue during the judging contest.

nirohgo commented:

Invalid because inspite of the observation being corrent there is no viable scenario where this would cause a meterial loss of funds. This would only affect the appraiser metrics with regards to non-pol bunnyToken funds held in the treasury (see _backing() function) and skew the value by univ3 fees that were not yet collected (way below 1% in most cases) plus any inaccuracy would be smooth potentially by use of additional feeds and moving average.