code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L742-L746

Vulnerability details

Impact

When dealing with baseToken with decimals over 36, price() in PrivatePool will revert, affecting deposit() in EthRouter that is externally calling price().

Proof of Concept

Here is the scenario:

ERC20(baseToken).decimals() = 38 exponent = 36 - 38 = -2 (leads to underflow and function reverts)

File:PrivatePool.sol#L742-L746

    function price() public view returns (uint256) {
        // ensure that the exponent is always to 18 decimals of accuracy
        uint256 exponent = baseToken == address(0) ? 18 : (36 - ERC20(baseToken).decimals());
        return (virtualBaseTokenReserves * 10 ** exponent) / virtualNftReserves;
    }

Consequently, EtnRouter.deposit() also reverts on line 233 of the code block below:

File: EthRouter.sol#L233-L236

233:        uint256 price = PrivatePool(privatePool).price();
        if (price > maxPrice || price < minPrice) {
            revert PriceOutOfRange();
        }

Tools Used

Manual

Recommended Mitigation Steps

Since exponentiation power is not allowed to be a signed integer type in Solidity, consider having the affected code block refactored with a different logic.

Here is a useful pure function that may be implemented in PrivatePool.sol:

  function scaleUnderlyingAmtTo18Decimals(
    uint256 _underlyingAmt,
    uint256 _underlyingTokenDecimals
  ) public pure returns (uint256) {
    return
      (_underlyingAmt * 1e18) /
      10**(_underlyingTokenDecimals);
  }

With this, price() may be refactored as follows:

    function price() public view returns (uint256) {
        // ensure that the exponent is always to 18 decimals of accuracy
-        uint256 exponent = baseToken == address(0) ? 18 : (36 - ERC20(baseToken).decimals());
-        return (virtualBaseTokenReserves * 10 ** exponent) / virtualNftReserves;
+        if (baseToken != address(0)) {
+            return scaleUnderlyingAmtTo18Decimals(virtualBaseTokenReserves, ERC20(baseToken).decimals()) / virtualNftReserves;
+        } else {
+            return (virtualBaseTokenReserves * 1e18) / virtualNftReserves;
+        }    
    }

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as primary issue

c4-sponsor commented 1 year ago

outdoteth marked the issue as sponsor acknowledged

GalloDaSballo commented 1 year ago

In contrast to low decimals issues, which are more common, very few if no token has 36 decimals, even safeMoon has 24

I also know that Yearn V2, which has been battle tested for over 2 years now, also doesn't support tokens with more than 36 decimals and have yet to see an issue with it

Am downgrading to QA for that reason

L

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

code-423n4 / 2023-04-caviar-findings