code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L750-L752 https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L632

Vulnerability details

Impact

FlashFee is calculated incorrectly as it does not consider the decimals of basetokens, and directly output the changeFee instead. It should be consistent with changeFeeQuote() method to calculate the real fee required to flash swap a given NFT.

Proof of Concept

In the privatePool.sol we have the flashFee function to calculate the flashloan fee to swap a given NFT. However, this function does not consider that basetokens may have different decimals rather than 18(like USDT for example) and it is inconsistent with the correct method applied in ChangeFeeQuote() function.

This calculation problem will result in either naive users lose tokens, or malicous attackers gain.

function flashFee(address, uint256) public view returns (uint256) {
        return changeFee;
    }

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;
    }

Tools Used

Manual review

Recommended Mitigation Steps

Consider using the correct method in ChangeFeeQuote to calculate flashFee. And remove unused parameters.

function flashFee() public view returns (uint256) {
        uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;
        uint256 feeAmount = changeFee * 10 ** exponent / 1e18;
    }

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as duplicate of #864

c4-judge commented 1 year ago

GalloDaSballo changed the severity to 2 (Med Risk)