0xmuxyz - The Market#`spendAllowance()` does not work properly when the Market#`borrow()` would be called via the MarketETHRouter#`borrow()` - due to lack of validation

[sherlock-admin2]

0xmuxyz

medium

The Market#spendAllowance() does not work properly when the Market#borrow() would be called via the MarketETHRouter#borrow() - due to lack of validation

Summary

The Market#spendAllowance() would be called in the Market#borrow().

However, the Market#spendAllowance() does not work properly when the Market#borrow() would be called via the MarketETHRouter#borrow() - due to lack of validation.

Vulnerability Detail

Within the MarketETHRouter#borrow(), the Market#borrow() would be called like this: https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/MarketETHRouter.sol#L73

  /// @notice Unwraps WETH from the floating pool and borrows to caller.
  /// @param assets amount of assets to borrow.
  /// @return borrowShares number of borrowed shares.
  function borrow(uint256 assets) external unwrap(assets) returns (uint256 borrowShares) {
    return market.borrow(assets, address(this), msg.sender);  ///<------------- @audit 
  }

Within the Market#borrow(), the Market#spendAllowance() would be called with a given borrower and assets like this: https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Market.sol#L145

  /// @notice Borrows a certain amount from the floating pool.
  /// @param assets amount to be sent to receiver and repaid by borrower.
  /// @param receiver address that will receive the borrowed assets.
  /// @param borrower address that will repay the borrowed assets.
  /// @return borrowShares shares corresponding to the borrowed assets.
  function borrow(
    uint256 assets,
    address receiver,
    address borrower
  ) external whenNotPaused whenNotFrozen returns (uint256 borrowShares) {
    spendAllowance(borrower, assets); ///<---------- @audit 
    ...

Within the Market#spendAllowance(), if the caller (msg.sender) and a given account would be different, the caller (msg.sender)'s allowance over account's assets would be checked and updated like this: https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Market.sol#L982-L985

  /// @notice Checks msg.sender's allowance over account's assets.
  /// @param account account in which the allowance will be checked.
  /// @param assets assets from account that msg.sender wants to operate on.
  function spendAllowance(address account, uint256 assets) internal {
    if (msg.sender != account) {  ///<-------@audit
      uint256 allowed = allowance[account][msg.sender]; // saves gas for limited approvals.　///<-------@audit

      if (allowed != type(uint256).max) allowance[account][msg.sender] = allowed - previewWithdraw(assets); ///<-------@audit
    }
  }

When a borrower would call the MarketETHRouter#borrow(), the caller (msg.sender) and the account of the Market#spendAllowance(), which is called in the Market#borrow() would be like this:

• The caller (msg.sender) would be the MarketETHRouter contract.
• The account would be the borrower.

In this case, within the Market#spendAllowance(), the previewWithdraw(assets) is supposed to be equal to or less than the allowed - if allowed != type(uint256).max:

If (allowed != type(uint256).max) allowance[account][msg.sender] = allowed - previewWithdraw(assets);

However, within the Market#spendAllowance(), since there is no validation to check whether or not a given assets would exceed the allowed (if allowed != type(uint256).max),

Hence, this allow the MarketETHRouter contract (msg.sender) to call the Market#borrow() with the amount (assets) of the asset, which is more than the amount of the shares-allowed.

This means that the Market#spendAllowance() does not work properly.

NOTE: By the way, the same situation would happen when the following situations:

• when the Market#borrowAtMaturity() would be called via the MarketETHRouter#borrowAtMaturity().
• when the Market#withdrawAtMaturity() would be called via the MarketETHRouter#withdrawAtMaturity().

Example scenario

Let's say Alice is a borrower.

• 1/ Alice would approve the MarketETHRouter contract to use 10 shares.

  • At this point, allowance[MarketETHRouter contract][Alice] == 10 shares.

• 2/ Alice would call the MarketETHRouter#borrow() with the amount (assets) of the asset, which is worth 100 shares.

  • At this point, the allowed in the Market#spendAllowance(), which is called inside the Market#borrow(), would be allowance[MarketETHRouter contract][Alice] == 10 shares.
  • In this case, the allowed (10 shares) is smaller than the previewWithdraw(assets) (100 shares).
  • Hence, her TX of the MarketETHRouter#borrow() is supposed to be reverted.

    If (allowed != type(uint256).max) allowance[account][msg.sender] = allowed - previewWithdraw(assets);

• 3/ However, since there is no validation to check whether or not a given assets would exceed the allowed, Alice's TX of the MarketETHRouter#borrow() would be successful.

  • As a result, she can borrow the amount (assets) of the asset, which is worth 100 shares - despite she approved the MarketETHRouter contract to use only 10 shares.

This means that the Market#spendAllowance() does not work properly.

NOTE: By the way, the same situation would happen when the following situations:

• when the Market#borrowAtMaturity() would be called via the MarketETHRouter#borrowAtMaturity().
• when the Market#withdrawAtMaturity() would be called via the MarketETHRouter#withdrawAtMaturity().

Impact

A borrower can borrow the amount of the asset, which is more than worth amount of the shares-allowed. This means that the Market#spendAllowance() does not work properly.

Code Snippet

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

Tool used

• Manual Review

Recommendation

Within the Market#spendAllowance(), consider adding a validation to check whether or not a given assets would equal to or less than the allowed (if allowed != type(uint256).max) like this:

  function spendAllowance(address account, uint256 assets) internal {
    if (msg.sender != account) {
      uint256 allowed = allowance[account][msg.sender]; // saves gas for limited approvals.

+     if (allowed != type(uint256).max) require(allowed >= previewWithdraw(assets), "The previewWithdraw(assets) must be equal to or less than the allowed");
      if (allowed != type(uint256).max) allowance[account][msg.sender] = allowed - previewWithdraw(assets);
    }
  }

[santipu03]

This issue is invalid because the transaction will revert when allowed < previewWithdraw(assets).