Caviar Private Pools - decade's results

Lines of code

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

Vulnerability details

Impact

-- A malicious owner of privatePool can keep track of all the approve() transaction that is required by traders in order to execute buy() transaction. -- Owner then frontruns these buy() transactions using execute() and directly transferring the approved NFT of the trader directly to him. -- The trader buy() transaction fails and also trader loses it's nft. -- This similar impact can be observed for underlying base ERC-20 token during sell() transaction. -- All external approves to the private pools are under risk and mercy of the owner.

Proof of Concept

1. In the execute function given below, a malicious user will set target argument as _nft contract itself, and data argument to be transferFrom(victim,owner,tokenId).
2. As soon as approve is called by the trader, the owner can go ahead with use the above mentioned function call to drain the traders.

    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();
        }
    }

Tools Used

-- Manual Review -- Foundry

Recommended Mitigation Steps

• Use of execute is a dangerous precedent under malicious owner. It is recommend to restrict this function as much as possible.
• execute function should not be able to call the base nft contract and base token contract.