Particle Protocol - Invitational - minhquanym's results

Lines of code

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

Vulnerability details

Impact

The withdrawEthWithInterest() function transfers ETH with interest back to the lender in case the loan is insolvent or the auction has concluded. It also transfers PnL to the borrower. However, if the borrower is a smart contract and rejects receiving ETH, the function will revert. This prevents the lender from withdrawing their ETH with interest.

The problem also exists in auctionBuyNft() function when it transfers PnL to borrower.

Proof of Concept

The withdrawEthWithInterest() function does a native ETH transfer to borrower.

function withdrawEthWithInterest(Lien calldata lien, uint256 lienId) external override validateLien(lien, lienId) {
  ...
  // transfer ETH with interest back to lender
  payable(lien.lender).transfer(lien.price + payableInterest);

  // transfer PnL to borrower
  if (lien.credit > payableInterest) {
      // @audit borrower can reject ETH, making it revert
      payable(lien.borrower).transfer(lien.credit - payableInterest); 
  }
  ...
}

The borrower contract can simply revert on receive() function to make it happen.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider applying "Pull over Push" patterns to mitigate the issue as described in https://fravoll.github.io/solidity-patterns/pull_over_push.html.

Assessed type

ETH-Transfer

Lines of code

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

Vulnerability details

Impact

In the Particle protocol, a portion of the lender's interest is kept by the treasury as described in the docs, and is also implemented in the _withdrawAccountInterest() function. However, in the withdrawEthWithInterest() function, the lender receives the entire payableInterest directly, which goes against the intended design.

Proof of Concept

The withdrawEthWithInterest() function simply transfers payableInterest plus the lien price directly to the lender.

function withdrawEthWithInterest(Lien calldata lien, uint256 lienId) external override validateLien(lien, lienId) {
  ...
  uint256 payableInterest = _calculateCurrentPayableInterest(lien);
  ...
  payable(lien.lender).transfer(lien.price + payableInterest); // @audit not apply treasury rate here
  ...
}

Tools Used

Manual Review

Recommended Mitigation Steps

Consider applying the treasury rate to the lender's interest in the withdrawEthWithInterest() function.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The function buyNftFromMarket() allows a borrower to buy an NFT from the same collection to repay a loan. At the end of the function flow, it checks that the contract actually holds the NFT tokenId and assumes that it is the acquired NFT. However, this is incorrect. The NFT tokenId might already be in the contract but belong to a different lien that is supplied by another lender. As a result, an attacker can use the funds that are supposed to buy tokenId to buy an arbitrary NFT from the marketplace and, at the same time, repay the lien that they are borrowing. At this moment, there are two liens tracking the same NFT tokenId, and whoever withdraws later will lose the NFT.

function _execBuyNftFromMarket(
  ...
  ) internal{
  ...
  // verify that the declared NFT is acquired and the balance decrease is correct
  // @audit tokenId might belong to other lien
  if (IERC721(collection).ownerOf(tokenId) != address(this) || balanceBefore - address(this).balance != amount) { 
      revert Errors.InvalidNFTBuy();
  }
}

Proof of Concept

Consider the scenario:

1. Alice (the attacker) is borrowing BAYC #111 from Bob (lender 1).
2. Caleb (lender 2) supplies BAYC #222 but has not been borrowed yet.
3. Alice calls buyNftFromMarket() to acquire a BAYC and close the active loan with Bob. However, she provides the tokenId = 222 and the tradeData is actually to buy a worthless NFT that she put to the marketplace, which is worth much less than a BAYC.
4. The result is that her low-value NFT is sold to the protocol with the price of a BAYC. She closed the loan with Bob.
5. Now, Bob and Caleb both have a lien tracking the same BAYC #222. The one who withdraws first will get it, and the other will lose the NFT.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a check to ensure that the contract does not hold the NFT before calling the marketplace.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

In the withdrawNftWithInterest() function, the lender can withdraw an NFT back if the NFT is currently in the contract without an active loan. However, the function makes an incorrect assumption that if the NFT can be withdrawn, then the loan is not active. This is a vulnerability because it allows the lender of a new lien to withdraw the NFT of an old lien that has been refinanced, even when the new lien is active.

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 
  
    // @audit not correct assumption
    // @audit it might belong to other lien
    IERC721(lien.collection).safeTransferFrom(address(this), msg.sender, lien.tokenId); 

    emit WithdrawNFT(lienId);

    // withdraw interest from this account too
    _withdrawAccountInterest(payable(msg.sender));
}

Proof of Concept

In the refinanceLoan() function, both liens have the same tokenId params, but that NFT should only be withdrawn by the old lien's lender, not the new lien's lender.

// update old lien
liens[oldLienId] = keccak256(
    abi.encode(
        Lien({
            lender: oldLien.lender,
            borrower: address(0),
            collection: oldLien.collection,
            tokenId: newLien.tokenId,
            price: oldLien.price,
            rate: oldLien.rate,
            loanStartTime: 0,
            credit: 0,
            auctionStartTime: 0
        })
    )
);

// update new lien
liens[newLienId] = keccak256(
    abi.encode(
        Lien({
            lender: newLien.lender,
            borrower: oldLien.borrower,
            collection: newLien.collection,
            tokenId: newLien.tokenId,
            price: newLien.price,
            rate: newLien.rate,
            loanStartTime: block.timestamp,
            credit: newCredit,
            auctionStartTime: 0
        })
    )
);

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a check in the withdrawNftWithInterest() function to ensure that the lien is not active.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The contract supports a "push-based" NFT supply, where the price and rate are embedded in the data bytes. This way, the lender doesn't need to additionally approve the NFT but can just transfer it directly to the contract. However, since the contract also interacts with the marketplace to buy/sell NFT, it has to prevent the issue where the marketplace also sends data bytes, which might tie 1 NFT with 2 different liens and create divergence.

function onERC721Received(
    address operator,
    address from,
    uint256 tokenId,
    bytes calldata data
) external returns (bytes4) {
    if (data.length == 64) {
        // @audit marketplace is router so the executor contract might not be whitelisted
        if (registeredMarketplaces[operator]) { 
            /// @dev transfer coming from registeredMarketplaces will go through buyNftFromMarket, where the NFT
            /// is matched with an existing lien (realize PnL) already. If procceds here, this NFT will be tied
            /// with two liens, which creates divergence.
            revert Errors.Unauthorized();
        }
        /// @dev MAX_PRICE and MAX_RATE should each be way below bytes32
        (uint256 price, uint256 rate) = abi.decode(data, (uint256, uint256));
        /// @dev the msg sender is the NFT collection (called by safeTransferFrom's _checkOnERC721Received check)
        _supplyNft(from, msg.sender, tokenId, price, rate);
    }
    return this.onERC721Received.selector;
}

It prevents it by using the registeredMarketplaces[] mapping, where it records the address of the marketplace. This check is explicitly commented in the codebase.

However, it is not enough. The protocol plans to integrate with Reservoir's Router contract, so only the Router address is whitelisted in registeredMarketplaces[]. But the problem is the address that transfers the NFT is not the Router, but the specific Executor contract, which is not whitelisted.

As a result, the marketplace might bypass this check and create a new lien in onERC721Received() during the buyNftFromMarket() flow, thus making 2 liens track the same NFT.

Proof of Concept

Function _execBuyNftFromMarket() does a low-level call to the exchange

// execute raw order on registered marketplace
bool success;
if (useToken == 0) {
    // use ETH
    // solhint-disable-next-line avoid-low-level-calls
    (success, ) = marketplace.call{value: amount}(tradeData);
} else if (useToken == 1) {
    // use WETH
    weth.deposit{value: amount}();
    weth.approve(marketplace, amount);
    // solhint-disable-next-line avoid-low-level-calls
    (success, ) = marketplace.call(tradeData);
}

The contract calls to Reservoir's router contract, which then calls to a specific module to execute the buy. https://github.com/reservoirprotocol/indexer/blob/6c89d546d3fb98d5eaa505b9943e89bd91f2e8ec/packages/contracts/contracts/router/ReservoirV6_0_1.sol#L50

function _executeInternal(ExecutionInfo calldata executionInfo) internal {
  address module = executionInfo.module;

  // Ensure the target is a contract
  if (!module.isContract()) {
    revert UnsuccessfulExecution();
  }

  (bool success, ) = module.call{value: executionInfo.value}(executionInfo.data);
  if (!success) {
    revert UnsuccessfulExecution();
  }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a flag that indicates the contract is in the buyNftFromMarket() flow and use it as a check in onERC721Received(). For example

_marketBuyFlow = 1;
_execBuyNftFromMarket(lien.collection, tokenId, amount, useToken, marketplace, tradeData);
_marketBuyFlow = 0;

And in onERC721Receive()

if (data.length == 64) {
  if(_martketBuyFlow) {
    return this.onERC721Received.selector;
  }
}

Assessed type

Invalid Validation

buyNftFromMarket(...) external payable override validateLien(Lien, LienId) nonReentrant {
 ...
}

if (data.length == 64) {
  if(_status === _ENTERED) {
    revert Errors.Unauthorized();
  }
}

Lines of code

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

Vulnerability details

The ParticleExchange contract does not store any data about the lien in the contract storage. Instead, users must send the entire Lien struct when interacting with any existing lien, and the contract checks if the hash of the struct is correct. This poses a problem because normal users must know the lien information when it is not stored on-chain.

function _validateLien(Lien calldata lien, uint256 lienId) internal view returns (bool) {
    return liens[lienId] == keccak256(abi.encode(lien));
}

As discussed with the sponsor on Discord, the lien information is stored in a database. A service job listens to on-chain events to update the database every minute on average.

On the other hand, the addCredit() function is public and allows anyone to add any amount of ETH to the credit of any active loan. This is where the issue occurs. Since users need to know the lien information when interacting with the protocol, an attacker can simply spam the addCredit() function with tiny amounts of credit. The purpose of this is to continuously change the lien information, causing other users' transactions to revert because they submit the wrong lien information. The issue can become a big problem during an auction. Lenders can make borrowers or other users unable to interact with the lien, allowing them to liquidate the loan.

Impact

The impact of this issue is a denial-of-service (DoS) attack during an auction. The attacker can keep changing the lien information, making it impossible for other users to interact with the lien.

Proof of Concept

Consider the scenario

1. Alice (attacker - lender) has supplied a NFT with price = 10 ETH but the floor price of it is only 7 ETH. So she wants to liquidate anyone borrow her NFT to sell her NFT for 10 ETH.
2. Bob (victim - borrower) took the offer of Alice and deposit 10 ETH + credit.
3. Alice calls startLoanAuction() as soon as possible to get that 10 ETH. During auction, she keeps calling addCredit() to the loan with tiny amount (1, 2 wei).
4. No normal users could interact with the lien to auctionBuyNft() or to repayWithNft() since the lien info keeps changing continously. After auction duration, Alice is able to withdraw 10 ETH even.

Tools Used

Manual Review

Recommended Mitigation Steps

There are 2 fixes for this issues

1. Adding a mininum requirement for addCredit() function.
2. Only allow borrowers to call addCredit() for their loan.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

In the protocol, lenders have to pay a small treasury fee when they claim their interest. The contract owner can change this _treasuryRate at any time using the function setTreasuryRate().

// @audit treasury rate should not affect existing loan
function setTreasuryRate(uint256 rate) external onlyOwner {
    if (rate > MathUtils._BASIS_POINTS) {
        revert Errors.InvalidParameters();
    }
    _treasuryRate = rate;
    emit UpdateTreasuryRate(rate);
}

However, when the admin changes the rate, the new treasury rate will also be applied to active loans, which is not the agreed-upon term between the lenders and borrowers when they supplied the NFT and created the loan.

Proof of Concept

Consider the following scenario:

1. Alice and Bob have an active loan with an accumulated interest of 1 ETH and _treasuryRate = 5%.
2. The admin suddenly changes the _treasuryRate to 50%. Now, if Alice claims the interest, she needs to pay 0.5 ETH to the treasury and keep 0.5 ETH.
3. Alice can either accept it and keep 0.5 ETH interest or front-run the admin transaction and claim before the _treasuryRate is updated. The point is that Alice only agreed to pay a 5% treasury rate at the beginning, so the new rate should not apply to her.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider storing the treasuryRate in the loan struct. The loan struct is not kept in storage, so the gas cost will not increase significantly.

Alternatively, consider adding a timelock mechanism to prevent the admin from changing the treasury rate.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In the function _execBuyNftFromMarket(), if the user chooses to use WETH, the function deposits ETH and approves the amount of WETH to the marketplace. After executing the trade at the marketplace, the function checks that the balance decrease is correct in the end. However, this check only accounts for ETH changes, not WETH changes, which is incorrect. If the trade did not use the full amount of WETH approved to the marketplace, some leftover WETH will remain in the contract. This amount of WETH/ETH will be locked in the contract, even though it should belong to the borrower who was able to get a good offer to buy the NFT at a lower price.

if (useToken == 0) {
    // use ETH
    // solhint-disable-next-line avoid-low-level-calls
    (success, ) = marketplace.call{value: amount}(tradeData);
} else if (useToken == 1) {
    // use WETH
    weth.deposit{value: amount}();
    weth.approve(marketplace, amount);
    // solhint-disable-next-line avoid-low-level-calls
    
    // @audit might not use all amount approved, cause ETH to locked in the protocol
    (success, ) = marketplace.call(tradeData); 
}

if (!success) {
    revert Errors.MartketplaceFailedToTrade();
}

// verify that the declared NFT is acquired and the balance decrease is correct
if (IERC721(collection).ownerOf(tokenId) != address(this) || balanceBefore - address(this).balance != amount) {
    revert Errors.InvalidNFTBuy();
}

Proof of Concept

Consider the following scenario:

1. Alice (borrower 1) calls buyNftFromMarket() to acquire an NFT with a price of 100 WETH. However, she sets the amount to 105 WETH, so the contract deposits and approves 105 WETH to the marketplace. After the trade, there is still 5 WETH approved to the marketplace.
2. Bob (borrower 2) sees the opportunity. He has a much cheaper lien, so he also calls buyNftFromMarket() to acquire an NFT with a price of 5 WETH. He specifies the useToken = 0. However, he sets the amount = 0 and actually uses the 5 WETH left in step 1 of Alice to acquire the NFT. The result is Bob is able to steal 5 WETH approved to the marketplace.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider accounting for the WETH when checking balance changes in _execBuyNftFromMarket().

Assessed type

Invalid Validation

Summary

L-1. Function repayWithNft() should not be called when the auction is concluded

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

Detail

A concluded auction allows the lender to liquidate the loan and take the ETH back. Thus, when the auction is concluded, the borrower should not be allowed to call repayWithNft().

Recommendation

Consider disabling repayWithNft() if the auction is concluded for that loan.

L-2. Lender can call stopLoanAuction() for a concluded auction

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

Detail

The lender can still call stopLoanAuction() for a concluded auction, while it should only be called for a live auction.

// @done-audit can stop a concluded auction
if (lien.auctionStartTime == 0) {
    revert Errors.AuctionNotStarted();
}

Recommendation

Consider disabling stopLoanAuction() for concluded auctions.

L-3. Anyone can create spam lien without transferring any ERC721 token

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

Detail

The function onERC721Received() accepts any sender, even EOA, to call it. Malicious users might spam call this function to create spam lien without transferring any ERC721 token. However, it will only affect the UI/UX of the protocol and not the smart contract if users do not interact with these malicious liens.

Recommendation

Consider disabling EOA to call onERC721Received(). It will not completely fix the issue since the contract can still be spammed, but it at least limits the possibility that users might mistakenly call this function.

L-4. Use of Floating Pragma

Occurrences

The floating pragma is used in the following locations:

• https://github.com/code-423n4/2023-05-particle/blob/1caf678bc20c24c96fc8f6b0046383ff0e9d2a6f/contracts/libraries/types/Structs.sol#L2
• https://github.com/code-423n4/2023-05-particle/blob/1caf678bc20c24c96fc8f6b0046383ff0e9d2a6f/contracts/protocol/ParticleExchange.sol#L2
• https://github.com/code-423n4/2023-05-particle/blob/1caf678bc20c24c96fc8f6b0046383ff0e9d2a6f/contracts/libraries/math/MathUtils.sol#L2

Detail

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

Recommendation

Consider using a fixed solidity version.

NC-1. Lacking Natspec comments

The following functions lack Natspec comments for their parameters:

• https://github.com/code-423n4/2023-05-particle/blob/1caf678bc20c24c96fc8f6b0046383ff0e9d2a6f/contracts/libraries/math/MathUtils.sol#L58
• https://github.com/code-423n4/2023-05-particle/blob/1caf678bc20c24c96fc8f6b0046383ff0e9d2a6f/contracts/protocol/ParticleExchange.sol#L790-L818

For example:

// @audit Missing bips in Natspec comment
/**
 * @dev Converts an integer bips value to a signed wad value.
 */
function bipsToWads(uint256 bips) public pure returns (uint256 wad) {
    return (bips * 1e18) / _BASIS_POINTS;
}

Recommendation

Consider completing the missing Natspec comments.

NC-2. Avoid magic values

Detail

The use of magic values should be avoided as they are easy to make mistakes with.

function calculateCurrentInterest(
    uint256 principal,
    uint256 rateBips,
    uint256 loanStartTime
) external view returns (uint256 interest) {
    uint256 loanTimeWad = (block.timestamp - loanStartTime) * 1e18; // @audit avoid magic value (1e18)
    interest = principal.wadMul(bipsToWads(rateBips).wadMul(loanTimeWad.wadDiv(_YEAR_WAD)));
    return interest;
}

Recommendation

Create constants for these values and use the constants instead of magic values.

Summary

G-1. Function withdrawEthWithInterest() calls transfer() for lender twice

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

Detail

Function withdrawEthWithInterest() does ETH transfers to lender twice in one transaction. It could be optimized to transfer only once.

payable(lien.lender).transfer(lien.price + payableInterest);

// transfer PnL to borrower
if (lien.credit > payableInterest) {
    payable(lien.borrower).transfer(lien.credit - payableInterest);
}

emit WithdrawETH(lienId);

// withdraw interest from this account too
_withdrawAccountInterest(payable(msg.sender));

Recommendation

ETH is transferred to lender in first line and last line in, which is twice. Consider grouping them into only 1 transfer.

G-2. internal functions only called once can be inlined to save gas

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

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

Detail

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

G-3. Optimize names to save gas

Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions.

Recommendation

Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas For example, the function IDs in the Core.sol contract will be the most used; A lower method ID may be given.

Proof of Consept

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

G-4. Use a more recent version of solidity

I recommend using the latest strictness version 0.8.19. Reference: https://blog.soliditylang.org/2023/02/22/solidity-0.8.19-release-announcement/