code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The pool owner can use execute to call any smartcontract function with pool as msg.sender. This allows them to steal any NFT or base token that was approved for the pool.

Proof of Concept

execute function allows owner to perform any function call from the pool:

/// @notice Executes a transaction from the pool account to a target contract. The caller must be the owner of the
/// pool. This allows for use cases such as claiming airdrops.
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();
    }
}

So any time a user or contract approves the pool to use their tokens (required for buy, sell and change) the owner can transfer those tokens to himself. This is not an issue when using periphery contract as intermediary to call the pool since the user does not directly approve the pool. However it poses a risk for custom contracts trying to integrate with the pool or users that want to use the change function directly (this one has no slippage so it has no explicit warning against direct use in the function documentation).

Tools Used

Manual review

Recommended Mitigation Steps

Either remove the execute function and make do without the possibility of claiming airdrops. Or document the issue clearly (risk of someone doing it wrong anyway)

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as duplicate of #184

c4-judge commented 1 year ago

GalloDaSballo changed the severity to 3 (High Risk)