code-423n4 / 2023-10-wildcat-findings

14 stars 10 forks source link

Lender can cause unintended behavior for the borrower's transaction #702

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L182-L190 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L112-L126

Vulnerability details

This vulnerability comes in the form of when a borrower wants to remove a lender as a both deposit and withdraw and set them as a withdraw only, to avoid paying more interest on their funds in the market, this plan may not go as planned, based on the nature of the lender you might be dealing with, A lender that wants to make the borrower pay absolute fee for their funds can simply frontrun this transaction to make them an only deposit lender, and deposit a large amount if the market is not in full capacity yet and refuse to withdraw has some sort of punishment or payback, it is really based on the lender behavior, malicious or non-malicious

Proof of Concept

Let's Explore how this will be carried out in practice

The borrower discovers that they don't want this Lender again in the market, as maybe they don't need the funds from them or they just cant pay that high interest rate based on the amount they put in the market, any reason the borrower does this is up to them.

They call the updateLenderAuthorization function to limit the actions of the lender, and make them only a withdrawal lender.

function updateLenderAuthorization(address lender, address[] memory markets) external {
    for (uint256 i; i < markets.length; i++) {
      address market = markets[i];
      if (!_controlledMarkets.contains(market)) {
        revert NotControlledMarket();
      }
      WildcatMarket(market).updateAccountAuthorization(lender, _authorizedLenders.contains(lender));
    }
  }

This calls the updateAccountAutorization function, The updateAccountAuthorization function, which is called from the first function, actually updates the authorization status based on the boolean value _isAuthorized. If the lender is authorized, they're given permission to deposit and withdraw; otherwise, they can only withdraw.

function updateAccountAuthorization(
    address _account,
    bool _isAuthorized
  ) external onlyController nonReentrant {
    MarketState memory state = _getUpdatedState();
    Account memory account = _getAccount(_account);
    if (_isAuthorized) {
      account.approval = AuthRole.DepositAndWithdraw;
    } else {
      account.approval = AuthRole.WithdrawOnly;
    }
    _accounts[_account] = account;
    _writeState(state);
    emit AuthorizationStatusUpdated(_account, account.approval);
  }

Monitoring the transactions taking place in the market on the mempool and seeing what their beloved borrower is about to do

The lender is not happy about this which means they cant put more money in the market and get paid the fees from it, they decide to frontrun the transaction and deposit a large sum into the account thus potentially increasing the interest the borrower have to pay to them, and they refuse to take their money from the vault

Lets create a Real Life Example of this happening

Alice is a borrower who has been using the Wildcat lending platform for her project. Over time, Bob, a lender, has been keeping a close eye on Alice's project and the transactions she makes on the platform. He notices that Alice frequently updates lender authorizations. One day, Alice decides to revoke Bob's deposit authorization as she's starting to believe that he's been aggressively depositing. Before she does this. Bob, having set up a monitoring mechanism, gets notified of a pending updateLenderAuthorization transaction from Alice that's aimed at revoking his deposit permissions. Seeing the opportunity. Bob immediately deposits a large amount of money into the market using a higher gas fee to ensure his transaction gets mined faster. Alice's transaction goes through a minute later, but by then, Bob's funds are already in, increasing the interest Alice has to pay.

Tools Used

VScode, Foundry

Recommended Mitigation Steps

The issue of frontrunning as being one of the core issue in DEFI, but here it takes a diffrent form as it depends on which kind of lender the borrower has in their markets, if not a quiet Lender then, the protocol is going to just inform the borrowers about all the things that can happen when executing transactions like this.

Assessed type

MEV

minhquanym commented 1 year ago

In summary, lender can front-run borrower to deposit before being removed. Seems like expected behavior

c4-pre-sort commented 1 year ago

minhquanym marked the issue as low quality report

c4-judge commented 1 year ago

MarioPoneder marked the issue as unsatisfactory: Insufficient proof