Caviar Private Pools - Emmanuel's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L256 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L331 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L442 https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L459-L476

Vulnerability details

Impact

Since end users will have to approve the PrivatePool contract to spend their erc20 tokens, or nfts before calling the buy(), sell(), or change() functions, malicious PrivatePool owner who sees the transactions in the mempool can frontrun those functions by calling the execute() function to illegally transfer some or all of the approved tokens to the contract. This would cause the user's call to be reverted because approved tokens for the operation is now less than the required price. To attract many users to interact with a PrivatePool, the malicious owner could make the price of the nft relatively cheap to attract buyers.

Proof of Concept

This is the execute function:

function execute(
        address target,
        bytes memory data
    ) public payable onlyOwner returns (bytes memory) {
        // call the target with the value and data
        (bool success, bytes memory returnData) = target.call{value: msg.value}(
            data
        );
        // if the call succeeded return the return data
        if (success) return returnData;
        // if we got an error bubble up the error message
        if (returnData.length > 0) {
            // solhint-disable-next-line no-inline-assembly
            assembly {
                let returnData_size := mload(returnData)
                revert(add(32, returnData), returnData_size)
            }
        } else {
            revert();
        }
    }

Here is a scenario:

• Attacker creates a PrivatePool with baseToken as USDC and Milady collection as nft, and sets price of each nft as 400USDC
• Alice has 500USDC, sees this PrivatePool selling Milady at a very cheap price, and then approves 400+USDC to the PrivatePool, and calls buy(tokenId, tokenWeight, proof), expecting to receive a Milady nft in exchange for USDC.
• Attacker sees the transactions in mempool, and calls PrivatePool's execute(USDCAddress,attackData) with higher gas; where attackData=abi.encodeWithSignature("transferFrom(address,address,uint)", AliceAddress, PrivatePoolAddress,400). This will pass because msg.sender in USDC.transferFrom() call is PrivatePool address(which has been approved by Alice).
• Now there is not enough approved tokens for Alice's buy() call, so transaction gets reverted.
• PrivatePool has now gained 400USDC without releasing any Milady nft, so Attacker may withdraw() the stolen funds later.
• Now, Attacker will create many more PrivatePools to get more victims.

NOTE: sell(), and change() functions are equally vulnerable.

Tools Used

Manual Review

Recommended Mitigation Steps

execute() function should be restricted or removed because it exposes many attack vectors.