elhaj - Precision Loss in `repayAtMaturity` can lead to system insolvency and loss of funds

[sherlock-admin3]

elhaj

high

Precision Loss in repayAtMaturity can lead to system insolvency and loss of funds

Summary

• A precision loss in the repayAtMaturity() function can lead to a gradual insolvency in the protocol, resulting in potential losses for depositors.

Vulnerability Detail

• The floatingBackupBorrowed variable is critical for maintaining the protocol's solvency as it tracks the amount of assets borrowed from the floating pool by fixed pools. Accurate tracking is vital since it influences key financial operations such as debt calculations, withdrawal limits, and share pricing.
• each time a borrower repaying at maturity causes a decrease in floatingBackupBorrowed. However, due to a rounding down issue in the scaleProportionally function, there's a consistent precision loss of 1 wei. This discrepancy means that the borrower's principal is reduced by 1 wei more than the amount subtracted from floatingBackupBorrowed and pool.borrowed.
• Over time, this small error can accumulate, leading to a scenario where the protocol believes there is debt remaining when, in fact, all borrowers have repaid their loans. This "phantom" debt affects the protocol's calculations, leading to incorrect interest rates, share prices ...ect .
• The precision loss originates from two functions, scaleProportionally and reduceProportionally, both using mulDivDown for calculations, which rounds down the results. When repaying, the principalCovered is calculated with scaleProportionally, which underestimates by 1 wei. Then, reduceProportionally is used to adjust the borrower's position, further reducing the principal by an additional wei due to rounding down.

 function scaleProportionally(Position memory position, uint256 amount) internal pure returns (Position memory) {
   uint256 principal = amount.mulDivDown(position.principal, position.principal + position.fee);
   position.principal = principal;
   position.fee = amount - principal;
   return position;
 }

and then we reduce the amount floatingBackupBorrowed and pool.borrowed by principal that we get from scaleProportionally function :

floatingBackupBorrowed -= pool.repay(principalCovered);

• when it comes to user position , we call reduceProportionally which also rounds down :

function reduceProportionally(Position memory position, uint256 amount) internal pure returns (Position memory) {
    uint256 positionAssets = position.principal + position.fee;
    uint256 newPositionAssets = positionAssets - amount;
    position.principal = newPositionAssets.mulDivDown(position.principal, positionAssets);
    position.fee = newPositionAssets - position.principal;
    return position;
  }

• in this case we can alway reduce the user position by 1wei more , Let's illustrate the precision loss issue with a step-by-step example:

• Initial state:

  • totalFloatingBorrowed = 1000 wei.
  • pool[1].borrowed = 1000 wei, pool[1].supplied = 0 wei.
  • User A's borrow position: userA[1].principal = 1000 wei, userA[1].fee = 100 wei.

User A decides to repay 100 wei:

1. Calculate principalCovered:

   • Using scaleProportionally:
     • principalCovered = (100 wei * 1000 wei) / (1000 wei + 100 wei) = 90 wei (rounded down).
   • Update floatingBackupBorrowed and pool[1].borrowed:
     • floatingBackupBorrowed = 1000 wei - 90 wei = 910 wei.
     • pool[1].borrowed = 910 wei.

2. Calculate User A's new position:

   • Using reduceProportionally:
     • New total position assets after repayment: 1100 wei - 100 wei = 1000 wei.
     • userPrincipal = (1000 wei * 1000 wei) / 1100 wei = 909 wei (rounded down).
     • userA[1].fee = 1000 wei - 909 wei = 91 wei.

Discrepancy:

• User A's principal is reduced by 91 wei, but the floatingBackupBorrowed and pool[1].borrowed are only reduced by 90 wei.

• Each repayment results in User A's principal being decreased by 1 wei more than the amount subtracted from floatingBackupBorrowed and pool.borrowed.

• This mismatch can lead to a growing discrepancy between the actual debt and the debt recorded by the protocol, potentially causing solvency issues.

• The reduceProportionally function should use the same principalCovered value to avoid this precision loss.

• The precision loss issue, although seemingly minor,it can be exploited by a malicious user who repeatedly repays a debt in increments of 1 wei. By creating a debt once and then partially repaying it, the user's principal is reduced by 1 wei more than what is subtracted from floatingBackupBorrowed and pool.borrowed. This can be repeated in a loop, with each repayAtMaturity call , to slowly but consistently deplete the protocol's assets. On Layer 2 networks, where gas fees are lower, such an attack becomes more feasible, especially with assets like WBTC that have fewer decimals and higher value, making the attack more impactful over time.

POC :

add this test :

  function test_precisionLoss() external {
   market.deposit(12 ether, address(this));
   // deposit
   market.borrowAtMaturity(FixedLib.INTERVAL, 1 ether, 1.5 ether, address(this), address(this));

   // get values before maturity  before :
   (uint256 principalBefore, uint256 feesBefore) = market.fixedBorrowPositions(FixedLib.INTERVAL, address(this));
   (uint borrowedBefore,,,) = market.fixedPools(FixedLib.INTERVAL);
   uint backUpBefore = market.floatingBackupBorrowed();
   // audit : repaying in a loop with 1 wei each time will decrease the position principal , but not borrowedbackup :
   for (uint256 i; i < 1000; i++) {
     market.repayAtMaturity(FixedLib.INTERVAL, 1, 1, address(this));
   }
   uint backUpAfter = market.floatingBackupBorrowed();
   ( uint borrowedAfter,,,) = market.fixedPools(FixedLib.INTERVAL);
   // get user maturity :
   (uint256 principalAfter, uint256 feesAfter) = market.fixedBorrowPositions(FixedLib.INTERVAL, address(this));
   assertEq(backUpAfter,backUpBefore);// floatingBackupBorrowed didn't change
   assertEq(borrowedAfter,borrowedBefore);// pool.borrowed didn't change
   assertEq(principalBefore - principalAfter , 1000);// fixedBorrowPositions[address(this)].principal reduced by 1000 (we loop 1000 time)

 }

Impact

• the increase in floatingBackupBorrowed will be locked forever in the contract even though it was paid it's not possible to withdraw it.
• incorrect shareToAssets calculation.
• Understates floatingBackupBorrowed, risking protocol insolvency.
• Depositors may not withdraw funds during liquidity shortages.
• Compromises system integrity, affecting debt and rate calculations.

Code Snippet

• scaleProportionally Function
• reduceProportionally Function
• repayAtMaturity Function

Tool used

Manual Review

Recommendation

• adjust the reduceProportionally function to use mulDivUp() for rounding up, ensuring consistency with the scaleProportionally function which rounds down.

 function reduceProportionally(Position memory position, uint256 amount) internal pure returns (Position memory) {
-   position.principal = newPositionAssets.mulDivDown(position.principal, positionAssets);
+   position.principal = newPositionAssets.mulDivUp(position.principal, positionAssets);
    position.fee = newPositionAssets - position.principal;
    return position;
}

[santipu03]

This issue is low (invalid) because it involves the loss of dust amounts of funds, which don't have any impact on the overall protocol behavior.

[0xumarkhatab]

The issue causes loss of funds . at least sounds clear to me.

Even the claim is backed by PoC

Accumulation of small dust can lead to a big value causing a lot of miscalculations in the rates .

[santipu03]

From the Sherlock guidelines:

The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

Your PoC shows a loss of 1e4 assets, that is 0.000000000001% of 1e18. Even if an attacker repeats for millions of times this attack, losing gas fees, it wouldn't even be significant.

[elhajin]

• @santipu03 The issue doesn't even require an attacker; it's a recurring problem with every 'repayAtMaturity' transaction, which keeps growing indefinitely. Also, relying solely on 1e18 is not always applicable. WBTC has 8 decimals and a high value. This, for sure, will lead to insolvency in the future for an active pool .

• The fact that #68 is valid with a proof of concept of an attacker iterating 20,000 times to steal $2400, ignoring the gas fees, and this, which grows indefinitely, not being valid, is weird .

[santipu03]

If we consider that for every repayment of a fixed loan, 1 wei of assets can become stuck in the pool, this may become a problem in the long run for the WBTC pool. A user could maybe cause a grief attack on the WBTC pool by executing this attack in a loop but it wouldn't make much sense because the gas fees would be high and there's no profit on doing it.

The main difference between this issue and 68 is that there's no profit in executing an attack like this, and the damage is more limited than 68. However, I admit that this issue can be triggered organically, even though the impact will be much more limited because it'd need a ton of fixed loan repayments.

After reconsidering all the facts, I believe this issue is borderline low/medium. The head of judging will have the final say on this.

[cvetanovv]

@elhajin This is a Low/Information severity issue. 1 wei is an extremely small value, even for 8 decimal tokens.

The attack vector you described in the issue is unrealistic, even for L2, where the gas is lower.

One transaction in optimism costs 0.1 gwei - https://tokentool.bitbond.com/gas-price/optimism

This is equal to 100000000 Wei. You can use this calculator to get an idea of how small 1 Wei is - https://eth-converter.com/