Jeiwan - Funds can be lost when repaying liens

[sherlock-admin]

Jeiwan

high

Funds can be lost when repaying liens

Summary

Funds can be lost when repaying liens

Vulnerability Detail

A lien can be repaid partially (LienToken.sol#L623-L629) or fully (LienToken.sol#L631-L642). When repaying partially, the whole paymentAmount is used. However, when repaying fully, the actual remaining debt can be smaller than paymentAmount, but full paymentAmount is always transferred from the payer (LienToken.sol#L645). This becomes critical when a borrower repays multiple liens by calling this makePayment function (LienToken.sol#L387-L389).

Example exploit scenario:

1. Alice has 2 liens: 1 ETH and 10 ETH (including accrued interest).
2. Alice calls the makePayment function and sets paymentAmount to 5 ETH, i.e. she expects to fully repay the first lien and partially repay the second lien.
3. Per-lien payments are made in this loop (LienToken.sol#L415-L423).
4. The first lien is repaid with paymentAmount = 5 ETH, and since the lien's debt is only 1 ETH, only 1 of the 5 ETH must be used to pay the lien. However, the whole 5 ETH are transferred from Alice (LienToken.sol#L631-L645).
5. The second lien doesn't get paid at all since the capital spent on the first lien (5 ETH) gets subtracted from the total amount (also 5 ETH) (LienToken.sol#L416-L422). Thus, paymentAmount becomes 0 and the next payment is skipped due to this check (LienToken.sol#L600-L602).

   Impact

   A borrower can lose funds when repaying liens.

   Code Snippet

   LienToken.sol#L387:

   function makePayment(uint256 collateralId, uint256 paymentAmount) public {
   _makePayment(collateralId, paymentAmount);
   }

LienToken.sol#L410:

function _makePayment(uint256 collateralId, uint256 totalCapitalAvailable)
  internal
{
  uint256[] memory openLiens = liens[collateralId];
  uint256 paymentAmount = totalCapitalAvailable;
  for (uint256 i = 0; i < openLiens.length; ++i) {
    uint256 capitalSpent = _payment(
      collateralId,
      uint8(i),
      paymentAmount,
      address(msg.sender)
    );
    paymentAmount -= capitalSpent;
  }
}

LienToken.sol#L594:

function _payment(
  uint256 collateralId,
  uint8 position,
  uint256 paymentAmount,
  address payer
) internal returns (uint256) {
  if (paymentAmount == uint256(0)) {
    return uint256(0);
  }

  uint256 lienId = liens[collateralId][position];
  Lien storage lien = lienData[lienId];
  uint256 end = (lien.start + lien.duration);
  require(
    block.timestamp < end || address(msg.sender) == address(AUCTION_HOUSE),
    "cannot pay off an expired lien"
  );

  address lienOwner = ownerOf(lienId);
  bool isPublicVault = IPublicVault(lienOwner).supportsInterface(
    type(IPublicVault).interfaceId
  );

  lien.amount = _getOwed(lien);

  address payee = getPayee(lienId);
  if (isPublicVault) {
    IPublicVault(lienOwner).beforePayment(lienId, paymentAmount);
  }
  if (lien.amount > paymentAmount) {
    lien.amount -= paymentAmount;
    lien.last = block.timestamp.safeCastTo32();
    // slope does not need to be updated if paying off the rest, since we neutralize slope in beforePayment()
    if (isPublicVault) {
      IPublicVault(lienOwner).afterPayment(lienId);
    }
  } else {
    if (isPublicVault && !AUCTION_HOUSE.auctionExists(collateralId)) {
      // since the openLiens count is only positive when there are liens that haven't been paid off
      // that should be liquidated, this lien should not be counted anymore
      IPublicVault(lienOwner).decreaseEpochLienCount(
        IPublicVault(lienOwner).getLienEpoch(end)
      );
    }
    //delete liens
    _deleteLienPosition(collateralId, position);
    delete lienData[lienId]; //full delete

    _burn(lienId);
  }

  TRANSFER_PROXY.tokenTransferFrom(WETH, payer, payee, paymentAmount);

  emit Payment(lienId, paymentAmount);
  return paymentAmount;
}

Tool used

Manual Review

Recommendation

When processing lien repayments, track the actual amount that was repaid for each lien.

Duplicate of #190