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

14 stars 10 forks source link

Borrowers can be forced to pay more interest than they planned for #675

Closed c4-submissions closed 12 months ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L238-L240 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L252-L254 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L134-L144

Vulnerability details

A borrower can be made to pay more annual interest, than what they have planned, by having an authorized actor directly transfer the underlying asset directly to the vault, this might seem counterintuitive as the protocol docs says that this doesnt affect the internal accounting of the protocol, but is considered as deposit or repayment back from the borrower, also this is what makes this manipulation possible, since the accrued interest occurs, on total assets in the market including the amount the borrower pays back into the vault, this makes a unautorized transfer to the vault transferred to the vault recognized as the borrowers repayment, in which they have to pay interest for.

Proof of Concept

Lets explore this

An attacker wants to cause pain to the borrower and dosent even have a financial motivation from this act, maybe just have problems with the borrower, the attacker sends a medium size amount of money that will have an effect on how much interest the borrower pays.

The money gets sent, the borrower isnt even aware and thinks it is just another lender that just deposit the money they have agreed to either offchain or somewhere.

The Other Lenders deposit more assets into the market and all amounts have being combined together as borrowable assets for the borrower, then interest occurs.

The borrower having planned how much they are going to pay based on the amount they expect in a range to be deposit pays more than expected for it.

Down Below, you can see the total asset used to calucate how much interest to be accured is the total asset in the market.

  function totalAssets() public view returns (uint256) {
    return IERC20(asset).balanceOf(address(this));
  }

Which also appiles throughtout the protocol, the boroower is being made to pay intrest on all total assets in the market no matter who deposits them, either the lender or themselves or our unauthorized actor, as evident in the below snippet of code

function borrowableAssets() external view nonReentrantView returns (uint256) {
    return currentState().borrowableAssets(totalAssets());
  }

This kind of issue also affects the borrower, when they try to reduce the max total supply of the market, it prevents them as the market does not allow the new total supply to be less than the total supply of assets in the market, this will create a temporary problem which the borrower has to deal with because of mostly outside interference

function setMaxTotalSupply(uint256 _maxTotalSupply) external onlyController nonReentrant {
    MarketState memory state = _getUpdatedState();

    if (_maxTotalSupply < state.totalSupply()) {
      revert NewMaxSupplyTooLow();
    }

    state.maxTotalSupply = _maxTotalSupply.toUint128();
    _writeState(state);
    emit MaxTotalSupplyUpdated(_maxTotalSupply);
  }

As shown up in that snippet of code an unaccounted-for amount of assets can lead to a miscalculation of how the borrower accounts for future plans of the market and a temporary DOS in an urgent situation.

Tools Used

VScode, Foundry

Recommended Mitigation Steps

There is actually no way to stop actors from doing a direct transfer, but there is a way for the protocol keepers to handle effects like that, using some methods may affect the accounting of the protocol as the protocol depends mostly on the total assets present in the market for numerous calculations, The users, in my opinion, should be made aware of Effects like this That can occur in their Markets.

Assessed type

DoS

c4-pre-sort commented 1 year ago

minhquanym marked the issue as sufficient quality report

laurenceday commented 1 year ago

Expected behaviour defined in documentation, not a finding.

c4-sponsor commented 1 year ago

laurenceday (sponsor) disputed

c4-judge commented 1 year ago

MarioPoneder changed the severity to QA (Quality Assurance)

c4-judge commented 12 months ago

MarioPoneder marked the issue as grade-c