Caviar Private Pools - ulqiorra's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L459

Vulnerability details

Impact

In PrivatePool, there are functions that assume the trader has approved the contract to spend their funds. For example, these functions are buy(), change(), and deposit():

• https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L256
• https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L423-L426
• https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L502

However, these functions do not reset the allowance based on the results of the trades. The allowance can be greater than the commission requires, up to infinity. For example, some DeFi projects on Ethereum Mainnet set a request on the frontend to sign a transaction for infinite allowance so that the user does not pay unnecessary fees next time they interact with the project.

The pool owner can use the execute() function to call ERC20(baseToken).transferFrom(victimAddress, maliciousOwner, anyAmountOfTokens) on behalf of the pool, which has the approval from the victim to transfer their funds:

• https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L459

It should be noted that there is no implemented router for PrivatePool with ERC-20 base token. Therefore, interaction with such private pools is currently possible only directly, which makes the importance of the discovery HIGH.

Recommended Mitigation Steps

It is recommended to remove the execute() function and reconsider the way airdrops are received, or to clear the trader's allowance at the end of the transactions.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L459

Vulnerability details

Impact

Ownership of the pool is determined through ownership of the Factory ERC721 NFT:

modifier onlyOwner() virtual {
    if (msg.sender != Factory(factory).ownerOf(uint160(address(this)))) {
        revert Unauthorized();
    }
    _;
}

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L127-L133

The current owner can trade their ownership NFT in exchange for funds comparable to those locked in the pool. Or it could be the owner of an empty pool who transferred their NFT to someone else, and that person decided to provide liquidity to it.

However, before doing so, a malicious owner can use the PrivatePool.execute() function to grant themselves unlimited approval to withdraw baseToken from the pool, as well as make themselves the NFT operator for this pool, giving them the ability to always withdraw any NFT from the pool:

privatePool.execute(
    pool.baseToken(),
    abi.encodeWithSignature(
        "approve(address,uint256)", 
        hackerAddress, 
        type(uint).max
    )
);

privatePool.execute(
    pool.nft(),
    abi.encodeWithSignature(
        "setApprovalForAll(address,bool)",
        hackerAddress,
        true
    )
)

Thus, the previous owner will be able to withdraw all the funds and NFTs from the pool, even if they are no longer the actual owner of the pool.

Recommended Mitigation Steps

In its current form, the execute() function is too dangerous and should be removed.

Factory contract is Owned. However, it is recommended to use a two-step ownership mechanism, which reduces the probability of making a mistake when transferring ownership: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/3b117992e17ae67fcd19ea2bd988f64520813cd1/contracts/access/Ownable2Step.sol