cleanup() does not properly handle debt repayment

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/PositionManager/PositionManager.sol#L89-L109

Vulnerability details

Impact

The cleanup(...) function in the PositionManager.sol contract is used to deposit remaining users assets back to ROE, repaying debt if any. However the users debt will not be repaid if the user has debt leading to loss of funds for the lenders and the project

Proof of Concept

When cleanup(...) is called it checks if there is an outstanding debt to repay, the first condition checks if amt <= debt but in a situation where the amount is less than the debt amt < debt it means that the debt is not fully repaid and the difference debt - amt is greater than zero but unfortunately this difference is not cached in storage for later repayment (as repay(...) only burns the equivalent debt token owed). Also, the amount repaid when amt < debt

  function cleanup(ILendingPool LP, address user, address asset) internal {
    ...
    if (amt > 0) {
      ...
      if ( debt > 0 ){
        if (amt <= debt ) {
          LP.repay( asset, amt, 2, user);
          return;
        }
        ...
      }
      // deposit remaining tokens
      LP.deposit( asset, amt, user, 0 );
    }
  }

is sent to the reserve on behalf of the user in exchange for overlying exchange tokens.

The Problem here is that the function does not capture a scenario where the amount repaid is less than the debt

Tools Used

Manual review

Recommended Mitigation Steps

• consider re-writing the logic in the first conditional block to ensure that the transaction reverts if amt < debt

  function cleanup(ILendingPool LP, address user, address asset) internal {
    ...
    if (amt > 0) {
      ...
      if ( debt > 0 ){
        require(amt >= debt, '...')

        LP.repay( asset, debt, 2, user);
        amt = amt - debt;

        // deposit remaining tokens
        if (amt > 0) {
          LP.deposit( asset, amt, user, 0 );
        }

    }
  }

Assessed type

Other

[141345]

invalid

the original logic is right

[c4-pre-sort]

141345 marked the issue as low quality report

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid