Caviar Private Pools - nobody2018's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/PrivatePool.sol#L461 https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L166 https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L244 https://github.com/code-423n4/2023-04-caviar/blob/main/src/EthRouter.sol#L270

Vulnerability details

Impact

Anyone can create a PrivatePool and become its owner. PrivatePool.execute can call any contract. The intention of this function is to allow for use cases such as claiming airdrops. However, a malicious owner can use this function to steal the nft and baseToken (ERC20) that are approved to PrivatePool. Such attacks are common, such as SushiSwap's RouterProcessor2 approval attack.

Proof of Concept

There are two scenarios:

1. In EthRouter.sol, the sell/deposit/change function will call nft.setApprovalForAll(pool, true) before interacting with pool, and the approval will not be revoked after the interaction. Once there is any nft in the EthRouter contract, the attacker can create pool and get EthRouter's approval to the pool by these functions.
2. PrivatePool is created by Factory, which belongs to the caviar protocol. Assume pool's baseToken is ERC20. Due to the trust in the caviar team, the external contract called nft.setApprovalForAll(pool, true) and baseToken.approve(pool, type(uint256).max) before interacting with pool. The approvals was not revoked after the interaction.

When the above scenario occurs, the owner of the pool can take away the nft/baseToken approved to the pool by calling pool.execute.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check for the target param in the PrivatePool.execute function.

function execute(
        address target,
        bytes memory data
    ) public payable onlyOwner returns (bytes memory) {
    	//+++++++++++++++++++++++++++++++++
        if (target == nft || target == baseToken) {
            revert InvalidTarget();
        }
        //+++++++++++++++++++++++++++++++++
        // call the target with the value and data
        (bool success, bytes memory returnData) = target.call{value: msg.value}(
            data
        );

        ......
    }