Particle Protocol - Invitational - adriro's results

Lines of code

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

Vulnerability details

Borrower can block being defaulted or auctioned

The borrower can potentially block the liquidation and auction processed by using a contract and reverting on ETH transfers.

Impact

When a loan is being liquidated or auctioned, any credit still available to the borrower (i.e. margin discounting the lender's interests) needs to be refunded. This is present in the withdrawEthWithInterest() function, that can be used to liquidate a position, and in the auctionBuyNft() function which is used to auction a loan.

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

192:     function withdrawEthWithInterest(Lien calldata lien, uint256 lienId) external override validateLien(lien, lienId) {
             ...
214:         // transfer PnL to borrower
215:         if (lien.credit > payableInterest) {
216:             payable(lien.borrower).transfer(lien.credit - payableInterest);
217:         }
             ...

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

688:     function auctionBuyNft(
689:         Lien calldata lien,
690:         uint256 lienId,
691:         uint256 tokenId,
692:         uint256 amount
693:     ) external override validateLien(lien, lienId) auctionLive(lien) {
             ...
741:         // pay PnL to borrower
742:         uint256 payback = lien.credit + lien.price - payableInterest - amount;
743:         if (payback > 0) {
744:             payable(lien.borrower).transfer(payback);
745:         }
             ...

As we can see in both of these cases, if the borrower still has some credit left, this is refunded by using the transfer() function. This is problematic, as the transfer() function reverts on failure. If the receiver is a contract, then this failure can be easily manipulated and forced.

This means that the borrower can use a contract to take the loan and arbitrarily decide to revert when the payback is being refunded, effectively blocking the liquidation and auction processes.

Proof of concept

The attacker can create a simple contract to interact with the Particle exchange. This contract has a receive() function that can be configured to revert.

function receive() external payable {
  if (_shouldRevert) {
    revert();
  }
}

The attacker then takes loans using this contract as an interface, and can block at will auctions or liquidations by setting _shouldRevert to true.

Recommendation

Similar to how interests are accrued for the lender, prefer a pull over push process to have the receiver pull ETH instead of pushing it in the middle of a transaction. The withdrawEthWithInterest() and auctionBuyNft() functions should accumulate amounts corresponding to the borrower in internal storage and then offer a function to withdraw these by separate.

Assessed type

DoS

Lines of code

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

Vulnerability details

Treasury fee is not collected in withdrawEthWithInterest()

The withdrawEthWithInterest() function fails to collect treasury fees from the lender interests.

Impact

The Particle exchange collects treasury fees from the lender's interests. These interests are accumulated in the interestAccrued mapping and are withdrawn using the _withdrawAccountInterest() function, which splits the portion that corresponds to the treasury.

https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L231-L246

231:     function _withdrawAccountInterest(address payable lender) internal {
232:         uint256 interest = interestAccrued[lender];
233:         if (interest == 0) return;
234: 
235:         interestAccrued[lender] = 0;
236: 
237:         if (_treasuryRate > 0) {
238:             uint256 treasuryInterest = MathUtils.calculateTreasuryProportion(interest, _treasuryRate);
239:             _treasury += treasuryInterest;
240:             interest -= treasuryInterest;
241:         }
242: 
243:         lender.transfer(interest);
244: 
245:         emit WithdrawAccountInterest(lender, interest);
246:     }

Lines 238-240 calculate treasury fees and accumulate them in the _treasury variable, which are later withdrawn by the owner using the withdrawTreasury() function.

However, these fees fail to be considered in the case of withdrawEthWithInterest():

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

192:     function withdrawEthWithInterest(Lien calldata lien, uint256 lienId) external override validateLien(lien, lienId) {
193:         if (msg.sender != lien.lender) {
194:             revert Errors.Unauthorized();
195:         }
196: 
197:         if (lien.loanStartTime == 0) {
198:             revert Errors.InactiveLoan();
199:         }
200: 
201:         uint256 payableInterest = _calculateCurrentPayableInterest(lien);
202: 
203:         // verify that the liquidation condition has met (borrower insolvent or auction concluded)
204:         if (payableInterest < lien.credit && !_auctionConcluded(lien.auctionStartTime)) {
205:             revert Errors.LiquidationHasNotReached();
206:         }
207: 
208:         // delete lien (delete first to prevent reentrancy)
209:         delete liens[lienId];
210: 
211:         // transfer ETH with interest back to lender
212:         payable(lien.lender).transfer(lien.price + payableInterest);
213: 
214:         // transfer PnL to borrower
215:         if (lien.credit > payableInterest) {
216:             payable(lien.borrower).transfer(lien.credit - payableInterest);
217:         }
218: 
219:         emit WithdrawETH(lienId);
220: 
221:         // withdraw interest from this account too
222:         _withdrawAccountInterest(payable(msg.sender));
223:     }

As we can see in the previous snippet of code, the interests are calculated in line 201 but that amount is then transferred, along with the lien price, back to the lender in full in line 212, without deducting any treasury fees.

Recommendation

The interest can be simply accumulated in the interestAccrued mapping, which are later withdrawn (correctly taking into account treasury fees) in the already present call to _withdrawAccountInterest().

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

      if (lien.loanStartTime == 0) {
          revert Errors.InactiveLoan();
      }

      uint256 payableInterest = _calculateCurrentPayableInterest(lien);

      // verify that the liquidation condition has met (borrower insolvent or auction concluded)
      if (payableInterest < lien.credit && !_auctionConcluded(lien.auctionStartTime)) {
          revert Errors.LiquidationHasNotReached();
      }

      // delete lien (delete first to prevent reentrancy)
      delete liens[lienId];
      
+     // accrue interest to lender
+     interestAccrued[lien.lender] += payableInterest;

@     // transfer ETH back to lender
@     payable(lien.lender).transfer(lien.price);

      // 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));
  }

Assessed type

Other

Judge has assessed an item in Issue #28 as 2 risk. The relevant finding follows:

[L-9] Griefer can DoS lender NFT withdrawals

Lines of code

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

Vulnerability details

Unspent WETH is not considered in buyNftFromMarket()

Impact

In the buyNftFromMarket() function, the borrower buys an NFT in order to repay and close their loan. The purchase is executed in the internal function named _execBuyNftFromMarket().

https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L395-L431

395:     function _execBuyNftFromMarket(
396:         address collection,
397:         uint256 tokenId,
398:         uint256 amount,
399:         uint256 useToken,
400:         address marketplace,
401:         bytes calldata tradeData
402:     ) internal {
403:         if (!registeredMarketplaces[marketplace]) {
404:             revert Errors.UnregisteredMarketplace();
405:         }
406: 
407:         uint256 balanceBefore = address(this).balance;
408: 
409:         // execute raw order on registered marketplace
410:         bool success;
411:         if (useToken == 0) {
412:             // use ETH
413:             // solhint-disable-next-line avoid-low-level-calls
414:             (success, ) = marketplace.call{value: amount}(tradeData);
415:         } else if (useToken == 1) {
416:             // use WETH
417:             weth.deposit{value: amount}();
418:             weth.approve(marketplace, amount);
419:             // solhint-disable-next-line avoid-low-level-calls
420:             (success, ) = marketplace.call(tradeData);
421:         }
422: 
423:         if (!success) {
424:             revert Errors.MartketplaceFailedToTrade();
425:         }
426: 
427:         // verify that the declared NFT is acquired and the balance decrease is correct
428:         if (IERC721(collection).ownerOf(tokenId) != address(this) || balanceBefore - address(this).balance != amount) {
429:             revert Errors.InvalidNFTBuy();
430:         }
431:     }

As we can see in the previous snippet of code, there are mainly two paths: use ETH or use WETH, which mainly depends on the specifics of the selected marketplace.

The ETH path is straightforward, the specified amount is sent as callvalue to the marketplace. If the difference doesn't match after the execution, because some ETH was refunded or something went wrong, it would cause a revert (line 428).

However, the WETH side is a bit different. Line 417 wraps the specified amount as WETH which is then approved to the marketplace. Now, if the marketplace doesn't consume the full amount, then any unused amount will still be present in the contract as WETH, causing the check in line 428 to succeed. In fact, if the intention is to use WETH (useToken == 1), the check in line 428 will always succeed as the amount parameter is transferred to the WETH contract causing balanceBefore - address(this).balance == amount to always be true.

Proof of concept

Let's say Bob wants to purchase an NFT to close his loan and submits an amount of 5 ether. The selected marketplace uses WETH so 5 ether are wrapped as WETH. The NFT is listed as 4.5 ETH and has an associated fee of 10%. The marketplace then pulls 4.95 WETH from the exchange contract. This leaves 0.05 WETH in the contract that is deducted from Bob's payback, while the check succeeds as the difference in ETH is still 5 ether.

Recommendation

The implementation can unwrap any available WETH after the execution of the marketplace operation so that it can be considered in the calculation present in the amount validation.

  function _execBuyNftFromMarket(
      address collection,
      uint256 tokenId,
      uint256 amount,
      uint256 useToken,
      address marketplace,
      bytes calldata tradeData
  ) internal {
      if (!registeredMarketplaces[marketplace]) {
          revert Errors.UnregisteredMarketplace();
      }

      uint256 balanceBefore = address(this).balance;

      // 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);
      }

      if (!success) {
          revert Errors.MartketplaceFailedToTrade();
      }
      
+     uint256 wethBalance = weth.balanceOf(address(this));
+     if (wethBalance > 0) {
+         weth.withdraw(wethBalance);
+     }

      // 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();
      }
  }

Assessed type

ERC20

Report

• Non Critical Issues (3)
• Low Issues (10)

Non Critical Issues

<a name="NC-1"></a>[NC-1] Use named parameters for mapping type declarations

Consider using named parameters in mappings (e.g. mapping(address account => uint256 balance)) to improve readability. This feature is present since Solidity 0.8.18

Instances (3):

File: contracts/protocol/ParticleExchange.sol

24:     mapping(uint256 => bytes32) public liens;

25:     mapping(address => uint256) public interestAccrued;

26:     mapping(address => bool) public registeredMarketplaces;

<a name="NC-2"></a>[NC-2] Unneeded explicit return

The explicit return can be omitted as the function is using named return variables.

Instances (1):

• https://github.com/code-423n4/2023-05-particle/blob/main/contracts/libraries/math/MathUtils.sol#L26

<a name="NC-3"></a>[NC-3] Use bool type for togglable parameter

Instances (1):

• https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L343

Low Issues

<a name="L-1"></a>[L-1] Contract files should define a locked compiler version

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

Instances (1):

File: contracts/protocol/ParticleExchange.sol

2: pragma solidity ^0.8.17;

<a name="L-2"></a>[L-2] _withdrawAccountInterest() can be front-runned to increase the treasury rate

https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L238

A call to _withdrawAccountInterest() can be front-runned to increase the treasury rate and diminish (or nullify) the lender's portion of the loan interests.

function _withdrawAccountInterest(address payable lender) internal {
    uint256 interest = interestAccrued[lender];
    if (interest == 0) return;

    interestAccrued[lender] = 0;

    if (_treasuryRate > 0) {
        uint256 treasuryInterest = MathUtils.calculateTreasuryProportion(interest, _treasuryRate);
        _treasury += treasuryInterest;
        interest -= treasuryInterest;
    }

    lender.transfer(interest);

    emit WithdrawAccountInterest(lender, interest);
}

The treasuryInterest variable is calculated as a proportion of the interest amount which is then subtracted to calculate the lender's share.

<a name="L-3"></a>[L-3] Relax amount check strictness while operating with marketplaces

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

In both sellNftToMarket() and buyNftFromMarket() the given amount is validated using a strict equality check.

These conditions can be relaxed to account for extra fees, rounding or potential minimal differences when the transaction gets executed. For example, the sell operation can check if the difference in balance is greater or equal than the amount (i.e. it received at least amount) and the buy operation can check the the balance differences is lower or equal than the amount (i.e. it sent at most amount).

<a name="L-4"></a>[L-4] The onERC721Received callback can be used to create fake liens

https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L74

Since the onERC721Received callback is a public function it can be called by anyone to create fake liens. The from parameter is user supplied to the function, which means that anyone can create fake liens on behalf of an arbitrary lender.

<a name="L-5"></a>[L-5] Accidental loss of NFTs due to misuse of push mechanism

https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L74

An accidental loss of NFTs can happen if the sender doesn't use safeTransferFrom() or submits an incorrect payload in safeTransferFrom().

<a name="L-6"></a>[L-6] auctionBuyNft() should use current auction price instead of amount parameter

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

The amount parameter in the auctionBuyNft() function should be used as a slippage check to ensure the caller gets at least an amount value from the action, but the effective value the offerer receives should be currentAuctionPrice (as this represents the maximum incentive the offerer gets while executing the action).

<a name="L-7"></a>[L-7] Use Ownable2Step instead of Ownable for access control

• https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L14

Use the Ownable2Step variant of the Ownable contract to better safeguard against accidental transfers of access control.

<a name="L-8"></a>[L-8] Provide safer limits for treasury rate

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

The current implementation of setTreasuryRate() only limits the rate parameter to _BASIS_POINTS, which if maxed represents the 100% of the lender's earnings.

<a name="L-9"></a>[L-9] Griefer can DoS lender NFT withdrawals

https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L172

A malicious actor can front-runs calls to withdrawNftWithInterest() to prevent the lender from claiming the NFT and deleting the lien, by taking a loan of the lien and withdrawing the NFT, causing the transaction to withdrawNftWithInterest() to fail. The malicious borrower can then immediately repay the loan with zero or minimum interests.

<a name="L-10"></a>[L-10] Marketplace calls are too permissive

• https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L316
• https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L414
• https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L420

The sellNftToMarket() and buyNftFromMarket() functions present in the ParticleExchange contract are used to execute a sell or a buy operation in a registered marketplace.

Even though marketplaces are whitelisted, both of these functions take an arbitrary tradeData payload that is supplied by the caller. This argument is then used as the calldata to the marketplace functions.

An attacker could use this to essentially call any function in the registered marketplace.

The recommendation here is to also whitelist function selectors and validate the right calls are being made. For example, this could be implemented as a mapping(address => mapping(bytes4 => bool)) that indicates whether a particular function signature is enabled for the corresponding marketplace.

MathUtils library

• Function calculateCurrentInterest() uses unnecessary intermediary WAD scaling resulting in unneeded multiplications and divisions as a result of wadMul and wadDiv.
  The calculation can be simplified as (principal * rateBps * (block.timestamp - loanStartTime)) / (_BASIS_POINTS * 365 days).
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/libraries/math/MathUtils.sol#L19

• Function calculateCurrentAuctionPrice() uses unnecessary intermediary WAD scaling resulting in unneeded multiplications and divisions as a result of wadMul and wadDiv.
  The calculation can be simplified as (price * auctionElapsed / auctionDuration.
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/libraries/math/MathUtils.sol#L36

• Function calculateTreasuryProportion() uses unnecessary intermediary WAD scaling resulting in unneeded multiplications and divisions as a result of wadMul and wadDiv.
  The calculation can be simplified as (interest * rateBips / _BASIS_POINTS.
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/libraries/math/MathUtils.sol#L50

ParticleExchange contract

• Storage variables _treasuryRate and _treasury could benefit from being packed together in a single slot as these are used in conjunction. This could be achieved by changing _treasuryRate to be a uint24 (which can safely fit the maximum rate of 100_000) and reducing the size of _treasury to uint232 (which can still represent a huge amount of ETH).
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L22-L23

• In withdrawEthWithInterest() there are potentially multiple calls to the lender to send ETH. Group these together in order to execute a single transfer() call.
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L212
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L222

• In the _withdrawAccountInterest() function the _treasuryRate variable is read twice from storage. Consider caching it locally to execute a single SLOAD.
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L237
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L238

• In the _withdrawAccountInterest() function, the math in _treasury += treasuryInterest can be unchecked as it would be practically impossible to overflow the size of the _treasury variable.
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L239

• In the _withdrawAccountInterest() function, the math in interest -= treasuryInterest can be unchecked as treasuryInterest is a portion of the interest (i.e. treasuryInterest <= interest).
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L240

• If marketplaces are trusted entities then WETH approval can be done once for an infinite amount instead of approving a specific amount in each call to _execBuyNftFromMarket().
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L418

• ERC721 transfers can be safely executed by calling transferFrom() instead of safeTransferFrom() when the recipient is the exchange contract in order to avoid the unneeded callback and save gas.
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L57
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L499
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L731

• In the stopLoanAuction() function the check lien.loanStartTime == 0 is unneeded this is already implied by the check below of lien.auctionStartTime == 0 (i.e. if lien.auctionStartTime != 0 is true then it already implies that the lien is taken).
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L659

• In the auctionBuyNft() function, the math in lien.credit + lien.price - payableInterest - amount can be unchecked as amount is lower than or equal to currentAuctionPrice, and currentAuctionPrice is a portion of maxSpendable (lien.credit + lien.price - payableInterest), which means that lien.credit + lien.price - payableInterest is safe (as it didn't overflow in line 699) and lien.credit + lien.price - payableInterest - amount is also safe as amount <= lien.credit + lien.price - payableInterest.
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L742

• In the withdrawTreasury() function, the _treasury storage variable is read twice from storage. Consider caching it locally to execute a single SLOAD.
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L809
  https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L813