code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Excess ETH for the flashloan fee is not refunded

Proof of Concept

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

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();

    // calculate the fee
    uint256 fee = flashFee(token, tokenId);

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

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

// calculate the fee
uint256 fee = flashFee(token, tokenId);

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

Here the msg.value could be more than fee. So remaining ETH should get refunded to the caller at then end of flashloan call.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider refunding remaining ETH to the caller at the end of the flashloan call like

if (msg.value > fee) {
    msg.value.safeTransferETH(msg.value - fee);
}

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as duplicate of #534

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

GalloDaSballo commented 1 year ago

4 Lows, awarding B because the quality of the reports is very high, recommend you send a QA next time as you may actually win in aggregate