sherlock-admin3 commented 6 months ago

Trumpero

medium

Inconsistency in `floatingAssets` updates in the Market contract

Summary

floatingAssets updates are inconsistent across different functions of the Market contract, and they do not calculate the exact floatingAssets for every operation.

Vulnerability Detail

In the Market contract, the beforeWithdraw() and afterDeposit() functions trigger updateFloatingDebt() before accrueAccumulatedEarnings() to update the floatingAssets storage. It does not accrue earnings from maturities to the floating pool.

function beforeWithdraw(uint256 assets, uint256) internal override whenNotPaused {
    updateFloatingAssetsAverage();
    depositToTreasury(updateFloatingDebt());
    uint256 earnings = accrueAccumulatedEarnings();
    uint256 newFloatingAssets = floatingAssets + earnings - assets;
    // check if the underlying liquidity that the account wants to withdraw is borrowed
    if (floatingBackupBorrowed + floatingDebt > newFloatingAssets) revert InsufficientProtocolLiquidity();
    floatingAssets = newFloatingAssets;
  }

  function afterDeposit(uint256 assets, uint256) internal override whenNotPaused whenNotFrozen {
    updateFloatingAssetsAverage();
    uint256 treasuryFee = updateFloatingDebt();
    uint256 earnings = accrueAccumulatedEarnings();
    floatingAssets += earnings + assets;
    depositToTreasury(treasuryFee);
    emitMarketUpdate();
  }

However, in functions at each maturity (such as borrowAtMaturity()), earnings from that specific maturity are accrued to update the floating assets. Earnings from other maturities and accumulated earnings are not accrued into floatingAssets during this function. Another point is that updateFloatingDebt() is called after earnings are accrued in borrowAtMaturity().

function borrowAtMaturity(
    uint256 maturity,
    uint256 assets,
    uint256 maxAssets,
    address receiver,
    address borrower
  ) external whenNotPaused whenNotFrozen returns (uint256 assetsOwed) {
  ...
  FixedLib.Pool storage pool = fixedPools[maturity];
  floatingAssets += pool.accrueEarnings(maturity);

  ...

  {
    uint256 backupDebtAddition = pool.borrow(assets);
    if (backupDebtAddition != 0) {
      uint256 newFloatingBackupBorrowed = floatingBackupBorrowed + backupDebtAddition;
      depositToTreasury(updateFloatingDebt());
      ...
    }
  }

Additionally, in the clearBadDebt() function, it calls accrueAccumulatedEarnings() before triggering updateFloatingDebt() in noTransferRefund(). This order is the opposite of the flow in beforeWithdraw() for updating floatingAssets.

function clearBadDebt(address borrower) external {
  if (msg.sender != address(auditor)) revert NotAuditor();

  floatingAssets += accrueAccumulatedEarnings();

  ...
  if (account.floatingBorrowShares != 0 && (accumulator = previewRepay(accumulator)) != 0) {
    (uint256 badDebt, ) = noTransferRefund(accumulator, borrower);
    totalBadDebt += badDebt;
  }

Impact

The inconsistent and imprecise floatingAssets may cause incorrect debt calculations and fluctuations in share prices and interest for users

Code Snippet

https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Market.sol#L698-L717 https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Market.sol#L293-L302 https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Market.sol#L622 https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Market.sol#L649

Tool used

Manual Review

Recommendation

When updating floatingAssets in updateFloatingDebt(), it should accrue earnings from the accumulator and maturities before calculating debt, to ensure an exact floatingAssets for every operation in the market.

itofarina commented 6 months ago

hey @santipu03 can we get a PoC demonstrating the impact?

santipu03 commented 6 months ago

request poc

sherlock-admin4 commented 6 months ago

PoC requested from @Trumpero

Requests remaining: 11

Trumpero commented 6 months ago

The following tests will compare the flow of updating floatingAsset in the clearBadDebt() function with the totalAssets() function. The totalAssets() function has the same flow as deposit and withdraw since it doesn't accrue accumulated earnings before calculating the interest rate in totalFloatingBorrowAssets().

function testClearBadDebtUpdatingFlow() external {
    //setup market
    marketWETH.setMaxFuturePools(3);
    irm = MockInterestRateModel(address(new MockBorrowRate(0.1e18)));
    market.setInterestRateModel(InterestRateModel(address(irm)));
    marketWETH.setInterestRateModel(InterestRateModel(address(irm)));

    daiPriceFeed.setPrice(50_000_000_000_000e18);
    market.deposit(100 ether, address(this));
    auditor.enterMarket(market);

    marketWETH.deposit(1_000 ether, BOB);
    market.borrowAtMaturity(FixedLib.INTERVAL, 10 ether, type(uint256).max, address(this), address(this));
    market.depositAtMaturity(FixedLib.INTERVAL, 100 ether, 0, address(this));
    assertGt(market.earningsAccumulator(), 0);

    vm.warp(block.timestamp + 1 weeks);

    //call clearBadDebt to update floatingAssets by accruing earnings from accumulator.
    vm.prank(address(market.auditor()));
    market.clearBadDebt(address(0));

    vm.warp(block.timestamp + 1 weeks);
    console.log(market.totalAssets());
    //100014792899408284023
  }

  function testNormalFlow() external {
    //setup market
    marketWETH.setMaxFuturePools(3);
    irm = MockInterestRateModel(address(new MockBorrowRate(0.1e18)));
    market.setInterestRateModel(InterestRateModel(address(irm)));
    marketWETH.setInterestRateModel(InterestRateModel(address(irm)));

    daiPriceFeed.setPrice(50_000_000_000_000e18);
    market.deposit(100 ether, address(this));
    auditor.enterMarket(market);

    marketWETH.deposit(1_000 ether, BOB);
    market.borrowAtMaturity(FixedLib.INTERVAL, 10 ether, type(uint256).max, address(this), address(this));
    market.depositAtMaturity(FixedLib.INTERVAL, 100 ether, 0, address(this));
    assertGt(market.earningsAccumulator(), 0);

    vm.warp(block.timestamp + 1 weeks);
    vm.warp(block.timestamp + 1 weeks);
    console.log(market.totalAssets());
    //100014285714285714285
  }

You can run these tests in Market.t.sol. The results of market.totalAssets() in these tests are different, causing unexpected changes in the market share price.

In the first test function, it calls clearBadDebt() with address(0) to only accrue the earnings from the market's accumulator, resulting in a different totalAsset() compared to the normal flow. Therefore, the updating of floatingAssets in clearBadDebt() is inconsistent with the deposit() and withdraw() functions, since they call updateFloatingDebt() to calculate interest before accruing the accumulator's earnings. Similarly, there is inconsistency in the updating flow of floatingAssets from other functions mentioned in the report.

santipu03 commented 6 months ago

@Trumpero The difference on totalAssets showed on the test is quite small, around ~0.0005e18. I get that the difference can be higher depending on the scenario, but is there a way that this difference can cause a loss of funds or disrupt the protocol functionality?

I need to see a PoC where this bug causes a loss of funds or disrupts the protocol functionality or I will close it.

santipu03 commented 6 months ago

@itofarina @cruzdanilo Trumpero is busy these days so confirm or dispute the issue because I will close it. Later in the escalations he can present the PoC if necessary.

santichez commented 6 months ago

@santipu03 We will dispute the issue.

Although what @Trumpero highlights about the inconsistency in the update flows of the floatingAssets variable is correct, to our extent this does not represent a risk. When calling functions at each maturity, earnings from other maturities are not accrued mainly due to gas cost reasons. The same occurs when interacting with floating functions; no maturity earnings are accrued in those cases. Moreover, the update of the floating debt being called after earnings are accrued on maturity operations was done on purpose. This was due to a bug that we fixed that was also explained in the post mortem of our last year's hack. We won't follow the recommendation since we don't want to accrue accumulated earnings every time the floating debt update is needed (i.e. borrow function) mainly due to gas costs reasons too. Regarding clearBadDebt, we are not okay with modifying the order of the accumulated earnings accrual since the fresh earningsAccumulator variable is needed to perform bad debt calculations, and if we perform an updateFloatingDebt() before, then it will be called anyways in the noTransferRefund. We acknowledge the inconsistency but we will not implement any fix and we highly appreciate the detailed explanation.

santipu03 commented 6 months ago

Agree with the sponsor

Trumpero commented 6 months ago

Escalate

Hi @santichez,

I just wrote a new POC to demonstrate the impact of the issue:

The test is based on the fact that in the functions mentioned in the report (in this case, I use the function withdrawAtMaturity), the floatingAssets are updated with the unassignedEarning before the function updateFloatingDebt() is invoked. Due to this order, the floatingAssets increase before we calculate the utilization in line 894. Hence, the utilization rate decreases, making the calculated floating interest less than the normal flow.

Here is the test:

function testIssueUsingWithdrawAtMaturity() external {
    // Setup market
    marketWETH.setMaxFuturePools(3);
    irm = MockInterestRateModel(address(new MockBorrowRate(0.1e18)));
    market.setInterestRateModel(InterestRateModel(address(irm)));
    marketWETH.setInterestRateModel(InterestRateModel(address(irm)));

    daiPriceFeed.setPrice(50_000_000_000_000e18);
    market.deposit(20 ether, address(this));
    auditor.enterMarket(market);

    marketWETH.deposit(1_000 ether, address(this));
    market.depositAtMaturity(FixedLib.INTERVAL, 1, 0, address(this));
    market.borrowAtMaturity(FixedLib.INTERVAL, 10 ether, type(uint256).max, address(this), address(this));
    market.borrow(1 ether, address(this), address(this));

    vm.warp(block.timestamp + 20 weeks);
    market.withdrawAtMaturity(FixedLib.INTERVAL, 1, 0, address(this), address(this));

    console.log("floatingDebt =", market.floatingDebt());
}

function testIssueNormalFlow() external {
    // Setup market
    marketWETH.setMaxFuturePools(3);
    irm = MockInterestRateModel(address(new MockBorrowRate(0.1e18)));
    market.setInterestRateModel(InterestRateModel(address(irm)));
    marketWETH.setInterestRateModel(InterestRateModel(address(irm)));

    daiPriceFeed.setPrice(50_000_000_000_000e18);
    market.deposit(20 ether, address(this));
    auditor.enterMarket(market);

    marketWETH.deposit(1_000 ether, address(this));
    market.depositAtMaturity(FixedLib.INTERVAL, 1, 0, address(this));
    market.borrowAtMaturity(FixedLib.INTERVAL, 10 ether, type(uint256).max, address(this), address(this));
    market.borrow(1 ether, address(this), address(this));

    vm.warp(block.timestamp + 20 weeks);
    market.deposit(1000, address(this));

    console.log("floatingDebt =", market.floatingDebt());
}

Log:

Ran 2 tests for test/Market.t.sol:MarketTest
[PASS] testIssueeNormalFlow() (gas: 1726147)
Logs:
  floatingDebt = 1038356164383561643

[PASS] testIssueeUsingWithdrawAtMaturity() (gas: 1661345)
Logs:
  floatingDebt = 1000000000000000000

Place these two tests within the file protocol > test > Market.t.sol to run. Running these tests, we observed that the floatingDebt in the first test remains unchanged after some time has passed, while in the second test, the floatingDebt increases by 3% after the same time interval. This demonstrates that the order of updating floatingAssets with unassignedEarning affects the floating interest.

The loss of floating interest is not limited to 3% as shown in the POC; it is based on these factors:

The larger the unassignedEarning, the lower the utilization, resulting in a larger loss of floating fee.
The longer the time passes without any interaction, the larger the loss of floating fee.

sherlock-admin3 commented 6 months ago

Escalate

Hi @santichez,

I just wrote a new POC to demonstrate the impact of the issue:

The test is based on the fact that in the functions mentioned in the report (in this case, I use the function withdrawAtMaturity), the floatingAssets are updated with the unassignedEarning before the function updateFloatingDebt() is invoked. Due to this order, the floatingAssets increase before we calculate the utilization in line 894. Hence, the utilization rate decreases, making the calculated floating interest less than the normal flow.

Here is the test:
function testIssueUsingWithdrawAtMaturity() external {
    // Setup market
    marketWETH.setMaxFuturePools(3);
    irm = MockInterestRateModel(address(new MockBorrowRate(0.1e18)));
    market.setInterestRateModel(InterestRateModel(address(irm)));
    marketWETH.setInterestRateModel(InterestRateModel(address(irm)));

    daiPriceFeed.setPrice(50_000_000_000_000e18);
    market.deposit(20 ether, address(this));
    auditor.enterMarket(market);

    marketWETH.deposit(1_000 ether, address(this));
    market.depositAtMaturity(FixedLib.INTERVAL, 1, 0, address(this));
    market.borrowAtMaturity(FixedLib.INTERVAL, 10 ether, type(uint256).max, address(this), address(this));
    market.borrow(1 ether, address(this), address(this));

    vm.warp(block.timestamp + 20 weeks);
    market.withdrawAtMaturity(FixedLib.INTERVAL, 1, 0, address(this), address(this));

    console.log("floatingDebt =", market.floatingDebt());
}

function testIssueNormalFlow() external {
    // Setup market
    marketWETH.setMaxFuturePools(3);
    irm = MockInterestRateModel(address(new MockBorrowRate(0.1e18)));
    market.setInterestRateModel(InterestRateModel(address(irm)));
    marketWETH.setInterestRateModel(InterestRateModel(address(irm)));

    daiPriceFeed.setPrice(50_000_000_000_000e18);
    market.deposit(20 ether, address(this));
    auditor.enterMarket(market);

    marketWETH.deposit(1_000 ether, address(this));
    market.depositAtMaturity(FixedLib.INTERVAL, 1, 0, address(this));
    market.borrowAtMaturity(FixedLib.INTERVAL, 10 ether, type(uint256).max, address(this), address(this));
    market.borrow(1 ether, address(this), address(this));

    vm.warp(block.timestamp + 20 weeks);
    market.deposit(1000, address(this));

    console.log("floatingDebt =", market.floatingDebt());
}
Log:
Ran 2 tests for test/Market.t.sol:MarketTest
[PASS] testIssueeNormalFlow() (gas: 1726147)
Logs:
  floatingDebt = 1038356164383561643

[PASS] testIssueeUsingWithdrawAtMaturity() (gas: 1661345)
Logs:
  floatingDebt = 1000000000000000000
Place these two tests within the file protocol > test > Market.t.sol to run. Running these tests, we observed that the floatingDebt in the first test remains unchanged after some time has passed, while in the second test, the floatingDebt increases by 3% after the same time interval. This demonstrates that the order of updating floatingAssets with unassignedEarning affects the floating interest.

The loss of floating interest is not limited to 3% as shown in the POC; it is based on these factors:

The larger the unassignedEarning, the lower the utilization, resulting in a larger loss of floating fee.

The longer the time passes without any interaction, the larger the loss of floating fee.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

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

santipu03 commented 6 months ago

In your PoC, if you add a normal deposit at the end of the test called testIssueeUsingWithdrawAtMaturity, then the floating debt is computed correctly. The PoC shows a 3% difference on the floating debt because it was stale until a new deposit/withdrawal occurred, and that is the expected behavior.

@Trumpero Is there anything more you'd like to add?

Trumpero commented 6 months ago

Ah yeah, that was my mistake when writting the POC. The utilization rate is different in 2 cases but it's not big enough to have a impact for the floatingDebt. You can close the escalation.

cvetanovv commented 6 months ago

We all agree it is invalid, so I will reject the escalation and leave the issue as is.

Evert0x commented 5 months ago

Result: Invalid Unique

sherlock-admin4 commented 5 months ago

Escalations have been resolved successfully!

Escalation status:

Trumpero: rejected

sherlock-audit / 2024-04-interest-rate-model-judging

Trumpero - Inconsistency in `floatingAssets` updates in the Market contract #203

Inconsistency in `floatingAssets` updates in the Market contract

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

sherlock-audit / 2024-04-interest-rate-model-judging

Trumpero - Inconsistency in `floatingAssets` updates in the Market contract #203

Inconsistency in floatingAssets updates in the Market contract

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

Inconsistency in `floatingAssets` updates in the Market contract