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#L638

Vulnerability details

Impact

NFT tokens could be permanently stolen by only borrowing one of those NFTs.

Proof of Concept

The function flashLoan() in PrivatePools.sol is responsible to execute a flash loan for a borrower. Its mechanism is such a way that sends the user a specific NFT id, and at the end of the function, the user has to repay that NFT id back plus some fee. The NFT transferring system is performed via Solmate's ERC721 safeTransferFrom() function:

ERC721(token).safeTransferFrom(address(this), address(receiver), tokenId);

The only restriction before the NFT transfer is checking the msg.value amount if the base token is the ETH address. As we can see from the tests, the function works well. However, there is a problem with the design pattern of the function. It indeed violates the checks-effects-interactions pattern and also lacks a reentrancy guard. If we look at Solmate's ERC721 contract we can see that the safeTransferFrom() is defined in the way below:

    function safeTransferFrom(
        address from,
        address to,
        uint256 id
    ) public virtual {
        transferFrom(from, to, id);

        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
    }

Thus any malicious contract which has the function onERC721Received() implemented in such a way that calls back the flashLoan() functions however with a different NFT id, will get some NFTs with just borrowing one NFT. Suppose this situation:

1 - Bob creates a smart contract that inherits from the ERC721TokenReceiver. 2 - He overrides the onERC721Received() function in such a way it calls the flashLoan() in a loop (e.g. 3 times) 3 - He gets some baseToken amounts to overcome the fee requirements. 4 - He now calls the flashLoan in another function like attack 5 - After these functions got executed, he receives three NFTs while he repays back the borrowed NFT

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider adding a non-reentrant modifier for flashLoan() or following the checks-effects-interactions pattern.

- function flashLoan(IERC3156FlashBorrower receiver, address token, uint256 tokenId, bytes calldata data)
        external
        payable
        returns (bool)
    {

+ function flashLoan(IERC3156FlashBorrower receiver, address token, uint256 tokenId, bytes calldata data)
        external
        payable
        nonReentrant
        returns (bool)
    {

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as duplicate of #685

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

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