TurnipBoy - _makePayment is logically inconsistent with how lien stack is managed causing payments to multiple liens to fail

[sherlock-admin]

TurnipBoy

medium

_makePayment is logically inconsistent with how lien stack is managed causing payments to multiple liens to fail

Summary

_makePayment(uint256, uint256) looping logic is inconsistent with how _deleteLienPosition manages the lien stack. _makePayment loops from 0 to openLiens.length but _deleteLienPosition (called when a lien is fully paid off) actively compresses the lien stack. When a payment pays off multiple liens the compressing effect causes an array OOB error towards the end of the loop.

Vulnerability Detail

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#_makePayment(uint256, uint256) loops from 0 to openLiens.Length. This loop attempts to make a payment to each lien calling _payment with the current index of the loop.

function _deleteLienPosition(uint256 collateralId, uint256 position) public {
  uint256[] storage stack = liens[collateralId];
  require(position < stack.length, "index out of bounds");

  emit RemoveLien(
    stack[position],
    lienData[stack[position]].collateralId,
    lienData[stack[position]].position
  );
  for (uint256 i = position; i < stack.length - 1; i++) {
    stack[i] = stack[i + 1];
  }
  stack.pop();
}

LienToken.sol#_deleteLienPosition is called on liens when they are fully paid off. The most interesting portion of the function is how the lien is removed from the stack. We can see that all liens above the lien in question are slid down the stack and the top is popped. This has the effect of reducing the total length of the array. This is where the logical inconsistency is. If the first lien is paid off, it will be removed and the formerly second lien will now occupy it's index. So then when _payment is called in the next loop with the next index it won't reference the second lien since the second lien is now in the first lien index.

Assuming there are 2 liens on some collateral. liens[0].amount = 100 and liens[1].amount = 50. A user wants to pay off their entire lien balance so they call _makePayment(uint256, uint256) with an amount of 150. On the first loop it calls _payment with an index of 0. This pays off liens[0]. _deleteLienPosition is called with index of 0 removing liens[0]. Because of the sliding logic in _deleteLienPosition lien[1] has now slid into the lien[0] position. On the second loop it calls _payment with an index of 1. When it tries to grab the data for the lien at that index it will revert due to OOB error because the array no long contains an index of 1.

Impact

Large payment are impossible and user must manually pay off each liens separately

Code Snippet

https://github.com/sherlock-audit/2022-10-astaria/blob/main/src/LienToken.sol#L410-L424

Tool used

Manual Review

Recommendation

Payment logic inside of AuctionHouse.sol works. _makePayment should be changed to mimic that logic.

[IAmTurnipBoy]

Escalate for 1 USDC

Not a dupe of #190, issue is with the lien array is managed as the payments are made. Fixing #190 wouldn't fix this.

[sherlock-admin]

Escalate for 1 USDC

Not a dupe of #190, issue is with the lien array is managed as the payments are made. Fixing #190 wouldn't fix this.

You've created a valid escalation for 1 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Evert0x]

Escalation accepeted