Caviar Private Pools - ck's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L385-L417 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L731-L738

Vulnerability details

Impact

When the PrivatePool::change() is called, fees are calculated using the changeFeeQuote function.

    function change(
        uint256[] memory inputTokenIds,
        uint256[] memory inputTokenWeights,
        MerkleMultiProof memory inputProof,
        IStolenNftOracle.Message[] memory stolenNftProofs,
        uint256[] memory outputTokenIds,
        uint256[] memory outputTokenWeights,
        MerkleMultiProof memory outputProof
    ) public payable returns (uint256 feeAmount, uint256 protocolFeeAmount) {
        // ~~~ Checks ~~~ //

        // check that the caller sent 0 ETH if base token is not ETH
        if (baseToken != address(0) && msg.value > 0) revert InvalidEthAmount();

        // check that NFTs are not stolen
        if (useStolenNftOracle) {
            IStolenNftOracle(stolenNftOracle).validateTokensAreNotStolen(nft, inputTokenIds, stolenNftProofs);
        }

        // fix stack too deep
        {
            // calculate the sum of weights for the input nfts
            uint256 inputWeightSum = sumWeightsAndValidateProof(inputTokenIds, inputTokenWeights, inputProof);

            // calculate the sum of weights for the output nfts
            uint256 outputWeightSum = sumWeightsAndValidateProof(outputTokenIds, outputTokenWeights, outputProof);

            // check that the input weights are greater than or equal to the output weights
            if (inputWeightSum < outputWeightSum) revert InsufficientInputWeight();

            // calculate the fee amount
            (feeAmount, protocolFeeAmount) = changeFeeQuote(inputWeightSum);
        }

    function changeFeeQuote(uint256 inputAmount) public view returns (uint256 feeAmount, uint256 protocolFeeAmount) {
        // multiply the changeFee to get the fee per NFT (4 decimals of accuracy)
        uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;
        uint256 feePerNft = changeFee * 10 ** exponent;

        feeAmount = inputAmount * feePerNft / 1e18;
        protocolFeeAmount = feeAmount * Factory(factory).protocolFeeRate() / 10_000;
    }

The issue is that when calcuating the exponent in the changeFeeQuote function, the subtraction ERC20(baseToken).decimals() - 4; will result in an underflow for tokens with less than 4 decimals.

Note that some popular tokens such as EURS with 2 decimals would fail. This means that the protocol would break when a user attempts to perform a change with tokens with less than 4 decimals.

Proof of Concept

Modify the test/shared/ShibaInu.sol file to set the token to 3 decimals:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "solmate/tokens/ERC20.sol";

contract ShibaInu is ERC20 {
    constructor() ERC20("Shiba Inu", "SHIB", 3) {}
}

Then run the test:

forge test -vvv --ffi --match-path test/PrivatePool/Change.t.sol

This will revert with the following error:

    ├─ [1393] PrivatePool::changeFeeQuote(3000000000000000000) [staticcall]
    │   ├─ [293] ShibaInu::decimals() [staticcall]
    │   │   └─ ← 3
    │   └─ ← "Arithmetic over/underflow"
    └─ ← "Arithmetic over/underflow"

Tools Used

Recommended Mitigation Steps

It may be advisable to use a scaling factor in the protocol when dealing with tokens to ensure the underflow does not happen