code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

In the PrivatePool contract, the flashFee is set equal to the changeFee which is set with 4 decimals of precision, but when the flashFee is applied in the flashLoan function it is not calculated correctly so the user who excute the flashloan will pay a very small fee not equal to what he was supposed to pay.

Proof of Concept

In the PrivatePool contract, the flashFee is set equal to the changeFee which is set with 4 decimals of precision, for example if changeFee == 25 and the baseToken is ETH then the actual charged fee is equal to (25/1e4) * 1e18 == 0.0025 ETH, this is explained in the code comments :

File: PrivatePool.sol Line 91

/// @notice The change/flash fee to 4 decimals of precision. For example, 0.0025 ETH = 25. 500 USDC = 5_000_000.
uint56 public changeFee;

When changeFee is used its value should always be converted using the formula :

(changeFee * 10**decimals) / 1e4

where decimals is the decimals value of the base token, note that this formula is used to convert the changeFee in the changeFeeQuote function.

Because flashFee is the same as changeFee, when flashFee is applied it must also use the aforementioned formula, but when the flashFee is used inside the flashLoan function the formula is not used instead the flashFee is used directly without conversion as it can been seen in the code below :

File: PrivatePool.sol Line 623-654

function flashLoan(IERC3156FlashBorrower receiver, address token, uint256 tokenId, bytes calldata data)
    external
    payable
    returns (bool)
{
    // check that the NFT is available for a flash loan
    if (!availableForFlashLoan(token, tokenId)) revert NotAvailableForFlashLoan();

    /** @audit
        fee is set equal to changeFee == flashFee(token, tokenId)
    */
    // calculate the fee
    uint256 fee = flashFee(token, tokenId);

    /** @audit
        fee is compared with msg.value directly which is incorrect
        as msg.value is in 18 decimals and fee is not
    */

    // if base token is ETH then check that caller sent enough for the fee
    if (baseToken == address(0) && msg.value < fee) revert InvalidEthAmount();

    // transfer the NFT to the borrower
    ERC721(token).safeTransferFrom(address(this), address(receiver), tokenId);

    // call the borrower
    bool success =
        receiver.onFlashLoan(msg.sender, token, tokenId, fee, data) == keccak256("ERC3156FlashBorrower.onFlashLoan");

    // check that flashloan was successful
    if (!success) revert FlashLoanFailed();

    // transfer the NFT from the borrower
    ERC721(token).safeTransferFrom(address(receiver), address(this), tokenId);

    /** @audit
        fee is transferred as if it has the same decimals as the baseToken but this is incorrect
    */
    // transfer the fee from the borrower
    if (baseToken != address(0)) ERC20(baseToken).transferFrom(msg.sender, address(this), fee);

    return success;
}

Because of this error the user who exute the flashloan call will pay a very small fee not equal to the actual one set in the contract.

For example if the baseToken is ETH and changeFee == 1 which means that changeFee is equal to 0.0001 * 10e18 which is in 18 decimals, but when the flashloan function is called the fee used will be equal to 1 thus equal to 10**(-18) which is very small.

Tools Used

Manual review

Recommended Mitigation Steps

Use the correct formula when applying the flashFee in the flashloan function, the flashloan function should be modified as follows :

function flashLoan(IERC3156FlashBorrower receiver, address token, uint256 tokenId, bytes calldata data)
    external
    payable
    returns (bool)
{
    // check that the NFT is available for a flash loan
    if (!availableForFlashLoan(token, tokenId)) revert NotAvailableForFlashLoan();

    /** @audit
        get flashFee == changeFee value with 4 decimals
    */
    // calculate the fee
    uint256 _flashFee = flashFee(token, tokenId);

    /** @audit
        calculate the correct flashFee value
    */
    uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;
    uint256 fee = _flashFee * 10 ** exponent;

    // if base token is ETH then check that caller sent enough for the fee
    if (baseToken == address(0) && msg.value < fee) revert InvalidEthAmount();

    // transfer the NFT to the borrower
    ERC721(token).safeTransferFrom(address(this), address(receiver), tokenId);

    // call the borrower
    bool success =
        receiver.onFlashLoan(msg.sender, token, tokenId, fee, data) == keccak256("ERC3156FlashBorrower.onFlashLoan");

    // check that flashloan was successful
    if (!success) revert FlashLoanFailed();

    // transfer the NFT from the borrower
    ERC721(token).safeTransferFrom(address(receiver), address(this), tokenId);

    // transfer the fee from the borrower
    if (baseToken != address(0)) ERC20(baseToken).transferFrom(msg.sender, address(this), fee);

    return success;
}

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)