NEW - Mitigation for H-02 and M-15 prevents private pool owner from performing some legitimate operations for `baseToken` and `nft` tokens owned by private pool

[code423n4]

Lines of code

https://github.com/outdoteth/caviar-private-pools/blob/main/src/PrivatePool.sol#L472

Vulnerability details

Impact

The mitigation for H-02: PrivatePool owner can steal all ERC20 and NFT from user via arbitrary execution and M-15: Pool tokens can be stolen via PrivatePool.flashLoan function from previous owner adds the following InvalidTarget error in the PrivatePool contract and updates the PrivatePool.execute function to execute if (target == address(baseToken) || target == address(nft)) revert InvalidTarget().

https://github.com/outdoteth/caviar-private-pools/pull/2/files#diff-6beea546a01e795e1365ca8a74b2bd952631f6cd228a5a869bfed82bf1f46a0fR80

error InvalidTarget();

https://github.com/outdoteth/caviar-private-pools/pull/2/files#diff-6beea546a01e795e1365ca8a74b2bd952631f6cd228a5a869bfed82bf1f46a0fR460-R479

    function execute(address target, bytes memory data) public payable onlyOwner returns (bytes memory) {
        if (target == address(baseToken) || target == address(nft)) revert InvalidTarget();

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

Because calling the PrivatePool.execute function when target is baseToken or nft now reverts, the private pool owner cannot use this function to transfer tokens of baseToken or nft for which users have approved the PrivatePool contract as an operator. Thus, H-02: PrivatePool owner can steal all ERC20 and NFT from user via arbitrary execution is mitigated. This change also mitigates M-15: Pool tokens can be stolen via PrivatePool.flashLoan function from previous owner since the private pool owner can no longer call the PrivatePool.execute function to approve other contract or EOA as the operator for the private pool's baseToken and nft tokens before becoming the previous owner of the same private pool.

However, this mitigation prevents the private pool owner from performing some legitimate operations for baseToken and nft tokens owned by the private pool. For example, if some rewards were accumulated for the baseToken tokens owned by the private pool, the private pool owner cannot call the PrivatePool.execute function to claim these rewards from the baseToken contract; in this case, the private pool owner loses these rewards that she or he is entitled to. Although the PrivatePool.execute function is meant for supporting the private pool owner's legitimate interactions with target, such functionality becomes unavailable for baseToken and nft after the mitigation.

Proof of Concept

The following steps can occur for the described scenario.

1. Alice is the owner of a private pool.
2. After some time, some rewards are accumulated for the baseToken tokens owned by her private pool in the corresponding baseToken contract.
3. Alice wants to claim these rewards. However, calling the PrivatePool.execute function to interact with the baseToken contract reverts.
4. Alice fails to claim these rewards that she is entitled to.

Tools Used

VSCode

Recommended Mitigation Steps

Instead of executing if (target == address(baseToken) || target == address(nft)) revert InvalidTarget(), the PrivatePool.execute function can be updated as follows.

1. The PrivatePool.execute function should revert if the private pool does not own the token ID, which is included in data, of the target contract.
2. An allowlist, which should only be updated by the trusted admin who is not the private pool owner, can be added for storing the safe and allowed function selectors for calling the PrivatePool.execute function for the corresponding private pool; unsafe function selectors like these for transfer, transferFrom, safeTransferFrom, approve, and setApprovalForAll should not be added to this list. This list can be empty at first and be updated on demand and by request of the private pool owner. Then, the PrivatePool.execute function should revert if the function selector, which is also included in data, is not found in this list.

If these changes require too much work, keeping the current mitigation also works because the benefit still outweighs the cost. Yet, users need to be aware of such design choice so they understand the limitation of being private pool owners.

Assessed type

Other

[c4-sponsor]

outdoteth marked the issue as sponsor acknowledged

[outdoteth]

Valid, but adds complexity which increases the surface area of attack for marginal benefit (the existing solution still mitigates the original issue).

[GalloDaSballo]

@outdoteth couldn't you just flashloan the token, do the operations and then return the token?

[GalloDaSballo]

I guess they can just withdraw and re-deposit, since they own the tokens since they own the pool

Am I missing something?

[outdoteth]

Ah yes I think you are right @GalloDaSballo

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

The Owner is free to operate with the tokens via macros, the change does add overhead but doesn't fundamentally impede any functionality, valid QA imo

[c4-judge]

GalloDaSballo marked the issue as grade-c

[GalloDaSballo]

Would rate as Low Severity, closing for awarding