0xAadi - Accounting Error in `lpToken0ToNativePrice()` and `lpToken1ToNativePrice()` Functions in `StrategyPassiveManagerVelodrome`

[sherlock-admin3]

0xAadi

high

Accounting Error in lpToken0ToNativePrice() and lpToken1ToNativePrice() Functions in StrategyPassiveManagerVelodrome

Summary

The lpToken0ToNativePrice() and lpToken1ToNativePrice() functions in the StrategyPassiveManagerVelodrome contract are designed to obtain the price of the LP tokens in native tokens. However, these functions fail to scale up the result, leading to a loss of precision.

Vulnerability Detail

The vulnerability lies in the following functions:

    /// @notice Returns the price of the first token in native token.
    function lpToken0ToNativePrice() public returns (uint256) {
@-->    uint amount = 10**IERC20Metadata(lpToken0).decimals() / 10;
        if (lpToken0 == native) return amount;
        return IQuoter(quoter).quoteExactInput(lpToken0ToNativePath, amount);
    }

    /// @notice Returns the price of the second token in native token.
    function lpToken1ToNativePrice() public returns (uint256) {
@-->    uint amount = 10**IERC20Metadata(lpToken1).decimals() / 10;
        if (lpToken1 == native) return amount;
        return IQuoter(quoter).quoteExactInput(lpToken1ToNativePath, amount);
    }

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L723C1-L735C6

The amount is scaled down by a factor of 10 when calculated from the decimals, but it is not scaled up when returned, causing precision loss.

Please see how StrategyPassiveManagerUniswap contract handled this

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/strategies/uniswap/StrategyPassiveManagerUniswap.sol#L720C1-L731C6

Impact

This miscalculation leads to precision loss and accounting errors in the contract due to getting a output amount scaled down by 10.

Code Snippet

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L725C1-L727C78

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L732C1-L734C78

Tool used

Manual Review

Recommendation

Update the lpToken0ToNativePrice() and lpToken1ToNativePrice() functions to scale the result correctly.

    /// @notice Returns the price of the first token in native token.
    function lpToken0ToNativePrice() public returns (uint256) {
        uint amount = 10**IERC20Metadata(lpToken0).decimals() / 10;
-       if (lpToken0 == native) return amount;
+       if (lpToken0 == native) return amount * 10;
-       return IQuoter(quoter).quoteExactInput(lpToken0ToNativePath, amount);
+       return IQuoter(quoter).quoteExactInput(lpToken0ToNativePath, amount) * 10; 
    }

    /// @notice Returns the price of the second token in native token.
-   function lpToken1ToNativePrice() public returns (uint256) {
+   function lpToken1ToNativePrice() public returns (uint256 amountOut) {
        uint amount = 10**IERC20Metadata(lpToken1).decimals() / 10;
-       if (lpToken0 == native) return amount;
+       if (lpToken0 == native) return amount * 10;
-       return IQuoter(quoter).quoteExactInput(lpToken1ToNativePath, amount);
+       return IQuoter(quoter).quoteExactInput(lpToken1ToNativePath, amount) * 10;
    }

[MirthFutures]

duplicate of issue 120. Low as this only for the UI and not used by the strategy for funds.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/beefyfinance/cowcentrated-contracts/pull/18

[sherlock-admin3]

Escalate

This report should be a valid medium and tagged as reward.

According to the Sherlock rules, the highest source of truth is:

Hierarchy of truth: If the protocol team provides no specific information, the default rules apply (judging guidelines). If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules.

For the Beefy Contest ReadMe, the sponsor answered the following question with Yes:

Q: Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold?

Yes

This issue demonstrates how the assumption that these two price retrieval functions will retrieve correct prices is incorrect and in reality they will return prices that are 10x too small. Whilst they are currently not utilised by the protocol for any internal calculations, if they were to be used in future integration in their current state, they could lead to massive undervaluations of token prices, which could lead to invalid mints, withdrawals, tvl calculations, and general losses.

The sponsor has already confirmed and fixed this issue, and with them stating they wish to receive issues regarding future integrations, this issue should be marked as valid and reward.

You've deleted an escalation for this issue.

[z3s]

Judging guide:

Future issues: Issues that result out of a future integration/implementation that was not mentioned in the docs/README or because of a future change in the code (as a fix to another issue) are not valid issues.

So, according to the guide, future integration must be documented. If you find a documented future feature where using lpToken0ToNativePrice() could cause a problem, then that could be a H/M issue.

[spacegliderrrr]

Fixed. Prices are now properly scaled.

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.