Caviar Private Pools - ych18's results

Lines of code

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

Vulnerability details

Impact

When the baseToken is ETH Native and even when providing a greater amount than the required fees for the various pools, a user cannot change his NFTs against different pools.

According to the System Overview, the EthRouter contract is responsible for taking in a sequence of actions and executing them against the various pools. However, this is not possible with the function EthRouter.change() In fact, when a user execute a change of NFTs against different pools. the function EthRouter.change() calls the PrivatePool.change()with the msg.value specified by the user when calling the function EthRouter.change(). Changing the NFTs for the first pool works perfectly, and this pool returns the remaining amount to the contract EthRouter. This amount is msg.value - feeAmount - protocolFeeAmount according to the line 436 of the PrivatePool. The second change call against the second pool reverts due to lack of fund. This is because the function EthRouter.change() tries to send to the second pool using the function PrivatePool.change() an ether value equal to msg.value. As the msg.value is used inside the loop ( which is another issue) and as some amount of this msg.value are sent to the first pool, the function reverts EthRouter.change() with OutOfFund. This is because the contract EthRouter tries to send msg.value to the second private, however this amount is not available.

Note

The only way to against a change against different PrivatePools is to send ether to theEthRouter directly and the execute the change().

Proof of Concept

Consider two private pools with ETH Native as baseToken. Calling the function ethRouter.change() to change NFTs against these two different privatePools reverts with OutOfFunds

##PoC_Test

    function test_ChangesDifferentPools() public {
        uint256[] memory inputTokenIds1 = new uint256[](5);
        uint256[] memory inputTokenWeights = new uint256[](0);
        uint256[] memory outputTokenIds1 = new uint256[](5);
        uint256[] memory outputTokenWeights = new uint256[](0);

        uint256[] memory inputTokenIds2 = new uint256[](5);
        uint256[] memory outputTokenIds2 = new uint256[](5);

        for (uint256 i = 0; i < 5; i++) {
            inputTokenIds1[i] = i + 10;
            outputTokenIds1[i] = i;
        }

        for (uint256 i = 0; i < 5; i++) {
            inputTokenIds2[i] = i + 15;
            outputTokenIds2[i] = i + 5;
        }

        EthRouter.Change[] memory changes = new EthRouter.Change[](2);
        changes[0] = EthRouter.Change({
            pool: payable(address(privatePool)),
            nft: address(milady),
            inputTokenIds: inputTokenIds1,
            inputTokenWeights: inputTokenWeights,
            inputProof: PrivatePool.MerkleMultiProof(
                new bytes32[](0),
                new bool[](0)
            ),
            stolenNftProofs: new IStolenNftOracle.Message[](0),
            outputTokenIds: outputTokenIds1,
            outputTokenWeights: outputTokenWeights,
            outputProof: PrivatePool.MerkleMultiProof(
                new bytes32[](0),
                new bool[](0)
            )
        });

        changes[1] = EthRouter.Change({
            pool: payable(address(privatePool2)),
            nft: address(milady),
            inputTokenIds: inputTokenIds2,
            inputTokenWeights: inputTokenWeights,
            inputProof: PrivatePool.MerkleMultiProof(
                new bytes32[](0),
                new bool[](0)
            ),
            stolenNftProofs: new IStolenNftOracle.Message[](0),
            outputTokenIds: outputTokenIds2,
            outputTokenWeights: outputTokenWeights,
            outputProof: PrivatePool.MerkleMultiProof(
                new bytes32[](0),
                new bool[](0)
            )
        });
        // act
        ethRouter.change{value: 100 ether}(changes, 0); // this reverts even if we specified 100 eth and more
    } 

Tools Used

Manuel Review

Recommended Mitigation Steps

Do not use msg.value inside a loop. Implement a solution like in EthRouter.buy() function with the baseTokenAmount as an argument of the function EthRouter.change(). Furthermore, add a sanity check that ensures that the sum of this baseTokenAmount is equal to msg.value