Skip to content

Latest commit

 

History

History
81 lines (59 loc) · 4.11 KB

M-05.md

File metadata and controls

81 lines (59 loc) · 4.11 KB
Raw

sell function in EthRouter doesn't check pool base token is ETH

Impact

The EthRouter contract can be used to execute batched actions to public or private pools. This contract operates using ETH, which means that the expected underlying base token of pools is ETH and not any ERC20 token.

It is possible for a user to perform an action on a pool with a base token other than ETH, due to a mistake or a misconfiguration. The buy function should be mostly safe as the pools verify that msg.value is zero if the base token is not ETH, which means that if a user tries to buy an NFT from a pool that is configured with an ERC20 token then the validation will revert the transaction.

However, things are different in the sell action. If the user executes an NFT sell using a pool with an ERC20 as the base token, then the call will succeed and the tokens will be sent to the EthRouter contract. As this contract is not prepared to deal with ERC20 tokens, then those tokens will be trapped in the contract. The user will transfer the NFT and won't receive any funds.

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

The scenario operates as follows (see PoC for a detailed walkthrough):

  1. User calls sell in EthRouter contract and uses a pool with an ERC20 as the base token.
  2. EthRouter transfers the NFT from to EthRouter contract
  3. EthRouter calls sell on the pool.
  4. Pool transfers the NFT from the EthRouter contract to the Pool contract, and transfers the resulting ERC20 tokens from the Pool to the EthRouter contract.
  5. Funds will be stuck in the EthRouter contract as there is no mechanism to withdraw them.

The error can be mitigated by the minOutputAmount parameter, which is the expected minimum ETH amount that the user will receive for the whole operation. However, in the case of multiple sell actions in the same call this parameter might not be too tight or be properly configured, and a single sell that goes wrong may be within the acceptable limit of minOutputAmount.

Proof of Concept

In the following test, Alice sells her NFT using a pool that's configured with an ERC20 token (MockToken). The call will succeed and the tokens will be stuck in the EthRouter contract.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_EthRouter_sell_DoesntValidatePoolUsesEth() public {
    // Setup pool with ERC20
    MockToken erc20 = new MockToken();
    PrivatePool privatePool = new PrivatePool(
        address(factory),
        address(royaltyRegistry),
        address(stolenNftOracle)
    );
    privatePool.initialize(
        address(erc20), // address _baseToken,
        address(milady), // address _nft,
        100e18, // uint128 _virtualBaseTokenReserves,
        10e18, // uint128 _virtualNftReserves,
        0, // uint56 _changeFee,
        0, // uint16 _feeRate,
        bytes32(0), // bytes32 _merkleRoot,
        false, // bool _useStolenNftOracle,
        false // bool _payRoyalties
    );
    erc20.mint(address(privatePool), 100e18);

    // Alice will mistakenly use the EthRouter to sell an NFT to the pool
    vm.startPrank(alice);

    uint256 tokenId = 0;
    milady.mint(alice, tokenId);

    EthRouter.Sell[] memory sells = new EthRouter.Sell[](1);
    sells[0].pool = payable(privatePool);
    sells[0].nft = address(milady);
    sells[0].tokenIds = new uint256[](1);
    sells[0].tokenIds[0] = tokenId;
    sells[0].isPublicPool = false;

    milady.setApprovalForAll(address(ethRouter), true);

    // The following action will succeed even though the pool is not using ETH as the base token
    ethRouter.sell(sells, 0, 0, false);

    // NFT will be transferred to pool
    assertEq(milady.ownerOf(tokenId), address(privatePool));

    // And tokens will be stuck in the EthRouter
    assertTrue(erc20.balanceOf(address(ethRouter)) > 0);

    vm.stopPrank();
}

Recommendation

The sell function should validate that each pool in the actions payload is using ETH as the base token, i.e. require(sells[i].pool.baseToken() == address(0)).