Treasury fee is not collected in `withdrawEthWithInterest()`

[code423n4]

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

[c4-judge]

hansfriese marked the issue as primary issue

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-sponsor]

wukong-particle marked the issue as sponsor acknowledged

[wukong-particle]

We will likely fix the issue in another way. We will modify withdrawNftWithInterest and withdrawEthWithInterest into withdrawNft and withdrawEth, i.e., move the interest withdraw into the single account level interest withdraw function (similar to the suggestion made in https://github.com/code-423n4/2023-05-particle-findings/issues/31)

[c4-judge]

hansfriese marked the issue as selected for report

[hansfriese]

After discussion, I think that HIGH is the appropriate severity because this issue incurs loss for the protocol.

[c4-judge]

hansfriese changed the severity to 3 (High Risk)

[wukong-particle]

Fixed with https://github.com/Particle-Platforms/particle-exchange-protocol/pull/10