Caviar Private Pools - minhtrng's results

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L459-L476

Vulnerability details

Impact

The pool owner can use execute to call any smartcontract function with pool as msg.sender. This allows them to steal any NFT or base token that was approved for the pool.

Proof of Concept

execute function allows owner to perform any function call from the pool:

/// @notice Executes a transaction from the pool account to a target contract. The caller must be the owner of the
/// pool. This allows for use cases such as claiming airdrops.
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();
    }
}

So any time a user or contract approves the pool to use their tokens (required for buy, sell and change) the owner can transfer those tokens to himself. This is not an issue when using periphery contract as intermediary to call the pool since the user does not directly approve the pool. However it poses a risk for custom contracts trying to integrate with the pool or users that want to use the change function directly (this one has no slippage so it has no explicit warning against direct use in the function documentation).

Tools Used

Manual review

Recommended Mitigation Steps

Either remove the execute function and make do without the possibility of claiming airdrops. Or document the issue clearly (risk of someone doing it wrong anyway)

Lines of code

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

Vulnerability details

Impact

EthRouter is meant to support multiple changes in one tx, but that would fail

Proof of Concept

The function EthRouter.change sends msg.value to pool in a for loop:

for (uint256 i = 0; i < changes.length; i++) {
    Change memory _change = changes[i];

    ...

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

The pool subtracts the fee, and sends the rest back to the router. After the first iteration the router contains less ETH than msg.value and will revert

<details> <summary>POC here</summary>

Add to Change.t.sol and run with forge test --match test_twoChanges -vvvv

    function test_twoChangesOneCall() public {
    uint256[] memory inputTokenIds = new uint256[](1);
    uint256[] memory inputTokenWeights = new uint256[](0);
    uint256[] memory outputTokenIds = new uint256[](1);
    uint256[] memory outputTokenWeights = new uint256[](0);

    uint256[] memory inputTokenIds2 = new uint256[](1);
    uint256[] memory inputTokenWeights2 = new uint256[](0);
    uint256[] memory outputTokenIds2 = new uint256[](1);
    uint256[] memory outputTokenWeights2 = new uint256[](0);

    inputTokenIds[0] = 5;
    outputTokenIds[0] = 0;

    inputTokenIds2[0] = 6;
    outputTokenIds2[0] = 1;

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

    changes[1] = EthRouter.Change({
        pool: payable(address(privatePool)),
        nft: address(milady),
        inputTokenIds: inputTokenIds2,
        inputTokenWeights: inputTokenWeights2,
        inputProof: PrivatePool.MerkleMultiProof(new bytes32[](0), new bool[](0)),
        stolenNftProofs: new IStolenNftOracle.Message[](0),
        outputTokenIds: outputTokenIds2,
        outputTokenWeights: outputTokenWeights2,
        outputProof: PrivatePool.MerkleMultiProof(new bytes32[](0), new bool[](0))
    });

    (uint256 changeFee,) = privatePool.changeFeeQuote(inputTokenIds.length * 1e18);

    //WARDEN: multiply with 10 just to make sure there really is enough
    ethRouter.change{value: changeFee*10}(changes, 0);

} 

Output:

...
    │   ├─ [0] PrivatePool::change{value: 50000000000000000000}([6], [], ([], []), [], [1], [], ([], [])) 
    │   │   └─ ← "EvmError: OutOfFund"
    │   └─ ← "EvmError: Revert"
    └─ ← "EvmError: Revert"

</details>

Tools Used

Manual review

Recommended Mitigation Steps

only send the required change fee and not msg.value

PrivatePool(_change.pool).change{value: _change.baseTokenAmount}(
  // -- snip --
);

Lines of code

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

Vulnerability details

Impact

flashFee is not scaled and returns a value that is too low. And hence fees paid are lower than they should be.

Proof of Concept

The view flashFee returns the storage variable changeFee which expresses an amount of baseTokens with 4 decimals:

function flashFee(address, uint256) public view returns (uint256) {
    return changeFee;
}

Correct scaling would be:

function changeFeeQuote(uint256 inputAmount) public view returns (uint256 feeAmount, uint256 protocolFeeAmount) {
    // multiply the changeFee to get the fee per NFT (4 decimals of accuracy)
    uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;
    uint256 feePerNft = changeFee * 10 ** exponent;

Tools Used

Manual review

Recommended Mitigation Steps

Adjust the view flashFee to use correct scaling

[01] missing check for msg.value if baseToken not ETH

PrivatePool.flashLoan does not check if ETH has been sent, even though baseToken is not ETH. The functions buy, change, deposit do perform this check:

if (baseToken != address(0) && msg.value > 0) revert InvalidEthAmount();

This could lead to users sending ETH to the contract on accident.

Mitigation: perform the check

[02] underflow for tokens with less than 4 decimals

Function PrivatePool.changeFeeQuote will revert if the baseToken has less than 4 decimals, rendering change function unusable:

function changeFeeQuote(uint256 inputAmount) public view returns (uint256 feeAmount, uint256 protocolFeeAmount) {

uint256 exponent = baseToken == address(0) ? 18 - 4 : ERC20(baseToken).decimals() - 4;

Tokens with less than 4 decimals are too rare for this to likely be an issue, but it could be fixed by adjusting the math (dividing by 10 ** (4-token.decimals) if decimals <= 3).

[03] owner can do MEV at cost of users

The functions setFeeRate and setVirtualReserves can be used by the owner to do MEV by frontrunning user transactions and adjusting the values to reach the users max slippage values.

Only viable mitigation would be removing them. But slippage is something that a user has to take into account anyway, so only a minor issue and likely users responsibility to use periphery properly.

[04] Documentation-implementation-mismatch for deposits and LPs

According to https://docs.caviar.sh/: "Custom Pools: [...] Like shared pools, liquidity providers earn fees from trades against their pool."

This is not the case in the current implementation (by design) and users might deposit in the believe of earning yield and being able to redeem, but would lose their deposits instead. Mitigation: update docs

[05] Function documentation misleading for EthRouter.buy

EthRouter.buy states:

/// @param payRoyalties Whether to pay royalties or not.

It might not be clear to users that private pools can not be opted out and the decision to pay royalties is taken by the pools owner. For a better user experience, consider reflecting this in the function documentation.

[06] obsolete comment

The function PrivatePool.deposit can not incur any slippage since it is not designed to mint LPs. Hence the following function documentation is obsolete:

/// @dev DO NOT call this function directly unless you know what you are doing. Instead, use a wrapper contract that
/// will check the current price is within the desired bounds.