Caviar Private Pools - BradMoon's results

Lines of code

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

Vulnerability details

Impact

Fund loss

Proof of Concept

Lines 254 to 293 have a vulnerability called "Dangerous Usage of msg.value inside a Loop". The function change accepts an array of Change struct as input and executes them one by one in a loop. Inside the loop, there is a call to PrivatePool.change function at line 273 which transfers ETH to the PrivatePool contract. The amount of ETH transferred is equal to msg.value which is set to the value of the value field in the transaction that calls the change function. Since the change function is called in a loop, the value of msg.value can be different for each iteration of the loop. This can lead to unexpected behavior and result in a loss of funds.

Tools Used

https://app.metatrust.io/

Recommended Mitigation Steps

To fix this vulnerability, we should move the transfer of ETH outside the loop and calculate the total amount of ETH to be transferred before calling the PrivatePool.change function. We can use a variable to keep track of the total amount of ETH to be transferred and update it inside the loop. Then, we can transfer the total amount of ETH to the PrivatePool contract after the loop.

Here is an example of the fixed code:

function change(Change[] calldata changes, uint256 deadline) public payable { // check deadline has not passed (if any) if (block.timestamp > deadline && deadline != 0) { revert DeadlinePassed(); }

uint256 totalValue = 0;

// loop through and execute the changes
for (uint256 i = 0; i < changes.length; i++) {
    Change memory _change = changes[i];

    // transfer NFTs from caller
    for (uint256 j = 0; j < changes[i].inputTokenIds.length; j++) {
        ERC721(_change.nft).safeTransferFrom(msg.sender, address(this), _change.inputTokenIds[j]);
    }

    // approve pair to transfer NFTs from router
    ERC721(_change.nft).setApprovalForAll(_change.pool, true);

    // calculate the value to be transferred
    uint256 changeValue = calculateChangeValue(_change);

    // add the value to the total value
    totalValue += changeValue;

    // execute change
    PrivatePool(_change.pool).change{value: changeValue}(
        _change.inputTokenIds,
        _change.inputTokenWeights,
        _change.inputProof,
        _change.stolenNftProofs,
        _change.outputTokenIds,
        _change.outputTokenWeights,
        _change.outputProof
    );

    // transfer NFTs to caller
    for (uint256 j = 0; j < changes[i].outputTokenIds.length; j++) {
        ERC721(_change.nft).safeTransferFrom(address(this), msg.sender, _change.outputTokenIds[j]);
    }
}

// transfer the total value to the PrivatePool contract
if (totalValue > 0) {
    (bool success, ) = address(PrivatePool).call{value: totalValue}("");
    require(success, "Transfer failed");
}

// refund any surplus ETH to the caller
if (address(this).balance > 0) {
    msg.sender.safeTransferETH(address(this).balance);
}

}

function calculateChangeValue(Change memory _change) internal view returns (uint256) { // calculate the value to be transferred uint256 inputValue = 0; for (uint256 i = 0; i < _change.inputTokenIds.length; i++) { inputValue += getTokenValue(_change.inputTokenIds[i], _change.inputTokenWeights[i]); }

uint256 outputValue = 0;
for (uint256 i = 0; i < _change.outputTokenIds.length; i++) {
    outputValue += getTokenValue(_change.outputTokenIds[i], _change.outputTokenWeights[i]);
}

return inputValue > outputValue ? inputValue - outputValue : 0;

}

function getTokenValue(uint256 tokenId, uint256 tokenWeight) internal view returns (uint256) { // calculate the value of the token // ... }