The Wildcat Protocol - rvierdiiev's results

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarket.sol#L142-L161

Vulnerability details

Proof of Concept

In case if market doesn't have enough balance then it's marked as delinquent. In this case if borrower will not transfer additional funds to cover needed funds, then after grace period, penalty will be applied, that borrower will have to pay.

FeeMath.updateTimeDelinquentAndGetPenaltyTime function is responsible to calculate penalty time that user should pay. This function is called every action along with _getUpdatedState function. So in case if user has any penalty time, then he should pay for it.

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/libraries/FeeMath.sol#L89-L123

  function updateTimeDelinquentAndGetPenaltyTime(
    MarketState memory state,
    uint256 delinquencyGracePeriod,
    uint256 timeDelta
  ) internal pure returns (uint256 /* timeWithPenalty */) {
    // Seconds in delinquency at last update
    uint256 previousTimeDelinquent = state.timeDelinquent;


    if (state.isDelinquent) {
      // Since the borrower is still delinquent, increase the total
      // time in delinquency by the time elapsed.
      state.timeDelinquent = (previousTimeDelinquent + timeDelta).toUint32();


      // Calculate the number of seconds the borrower had remaining
      // in the grace period.
      uint256 secondsRemainingWithoutPenalty = delinquencyGracePeriod.satSub(
        previousTimeDelinquent
      );


      // Penalties apply for the number of seconds the market spent in
      // delinquency outside of the grace period since the last update.
      return timeDelta.satSub(secondsRemainingWithoutPenalty);
    }


    // Reduce the total time in delinquency by the time elapsed, stopping
    // when it reaches zero.
    state.timeDelinquent = previousTimeDelinquent.satSub(timeDelta).toUint32();


    // Calculate the number of seconds the old timeDelinquent had remaining
    // outside the grace period, or zero if it was already in the grace period.
    uint256 secondsRemainingWithPenalty = previousTimeDelinquent.satSub(delinquencyGracePeriod);


    // Only apply penalties for the remaining time outside of the grace period.
    return MathUtils.min(secondsRemainingWithPenalty, timeDelta);
  }

Penalty time is calculated in 2 ways. It depends on market state: if it's delinquent or not. In case if yes, that means that user still didn't pay all funds and he should pay penalty for the time that is bigger than grace period. But in case if market is not delinquent anymore and state.timeDelinquent is not 0, it means that market was delinquent, but recently user has covered needed liquidity. So in this case user still should pay additional penalty until his state.timeDelinquent is less than grace period. And each time state.timeDelinquent is decreased with passed time.

As example consider next one. Market has 5 days grace period. Market became delinquent and after 7 days borrower recovered liquidity. So at that moment, when he called _getUpdatedState function, penalty of 2 days were applied. Now market is delinquent, but user still have to pay penalty for next 2 days(as timer now goes back) until he reaches grace period. So if someone calls any action on day 8, that means that borrower still should pay penalty for 1 day, and he will do that later, not now.

Now when we know this let's talk about closing of market. https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarket.sol#L142-L161

  function closeMarket() external onlyController nonReentrant {
    MarketState memory state = _getUpdatedState();
    state.annualInterestBips = 0;
    state.isClosed = true;
    state.reserveRatioBips = 0;
    if (_withdrawalData.unpaidBatches.length() > 0) {
      revert CloseMarketWithUnpaidWithdrawals();
    }
    uint256 currentlyHeld = totalAssets();
    uint256 totalDebts = state.totalDebts();
    if (currentlyHeld < totalDebts) {
      // Transfer remaining debts from borrower
      asset.safeTransferFrom(borrower, address(this), totalDebts - currentlyHeld);
    } else if (currentlyHeld > totalDebts) {
      // Transfer excess assets to borrower
      asset.safeTransfer(borrower, currentlyHeld - totalDebts);
    }
    _writeState(state);
    emit MarketClosed(block.timestamp);
  }

Market can be closed only when state.totalDebts() amount of assets is present in the market. state.totalDebts() is normalized total supply + unclaimed assets + protocol fees.

This means that market should be in delinquent state. However, there is not check that state.timeDelinquent <= delinquencyGracePeriod, which means that not whole penalty is applied yet. So borrower can close market and avoid repaying all penalty.

Impact

Borrower can close market without repaying penalty

Tools Used

VsCode

Recommended Mitigation Steps

Add check that state.timeDelinquent <= delinquencyGracePeriod when close market.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarket.sol#L142

Vulnerability details

Proof of Concept

WildcatMarket.closeMarket function allows borrower to close his market, so he repays everything and interests stop accruing. As you can see currently it can be called by controller only. But the problem is that WildcatMarketController doesn't have such functionality.

Impact

Borrower will not be able to close market and will continue pay interests.

Tools Used

VsCode

Recommended Mitigation Steps

Implement such function inside WildcatMarketController.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketConfig.sol#L134

Vulnerability details

Proof of Concept

WildcatMarketConfig.setMaxTotalSupply function allows borrower to change max total supply. New value can be only bigger than current total supply. As you can see currently it can be called by controller only. But the problem is that WildcatMarketController doesn't have such functionality.

Impact

Borrower will not be able to change total supply, so he will not be able to get additional liquidity into the market.

Tools Used

VsCode

Recommended Mitigation Steps

Implement such function inside WildcatMarketController.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketConfig.sol#L74-L81

Vulnerability details

Proof of Concept

In order if lender is malicious in any way, then WildcatMarketConfig.nukeFromOrbit can be called. This function will check that lender is sanctioned. And if so, then _blockAccount function is called.

This function will send all tokens of sanctioned user to the escrow. But, it will do that only if address is not already blocked. Note, that address will be marked as blocked, once this check will pass. Then if user has balance it will be sent to the escrow balance.

This should be a good protection that should take funds out of sanctioned user. However, it's not good enough as user can just frontrun nukeFromOrbit and transfer all his funds to another address(suppose he controls several approved addresses) or he can transfer it to any other authorised lender and then find a way to withdraw(maybe pay commission). This is possible, because transfer function doesn't check that from or to is sanctioned and allows to do transfer to any address. Instead this function calls _getAccount function to receive to and from accounts. But _getAccount function, only check that account is already blocked and not sanctioned. So frontrun will work.

Impact

Sanctioned user can avoid blocking and get all funds.

Tools Used

VsCode

Recommended Mitigation Steps

Do not allow to transfer market tokens from sanctioned address and to sanctioned address. Maybe make _getAccount function check if address is sanctioned instead of blocked.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketConfig.sol#L121 https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketWithdrawals.sol#L81

Vulnerability details

Proof of Concept

Wildcat protocol would like only authorized lender to be able to work with market. So borrower has ability to add/remove lenders. Using WildcatMarketController.updateLenderAuthorization anyone can trigger from controller to market, which will update lender's status for the market. This will just send true/false depending on lender's authorization.

As result, function will update account.approval to DepositAndWithdraw if authorized, or WithdrawOnly otherwise. The idea of updateAccountAuthorization function was to add ability to remove someone, who previously was authorized. But also it can be used by anyone to grant WithdrawOnly approval to any non-blocked address.

Once it's done, then that account has ability to withdraw from market and as result invariant that not allowed lender can't participate with market is broken.

Impact

Account that was not allowed by borrower to participate in market is able to do that.

Tools Used

VsCode

Recommended Mitigation Steps

Maybe better authorizeLenders and deauthorizeLenders function call market.updateAccountAuthorization for added/removed user.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketBase.sol#L172-L176 https://github.com/code-423n4/2023-10-wildcat/blob/main/src/WildcatSanctionsEscrow.sol#L38

Vulnerability details

Proof of Concept

In case if lender is sanctioned, then he can be blocked and his funds should go to the special escrow account. In order to create escrow WildcatSanctionsSentinel.createEscrow is called. It expects 3 params: borrower, account, asset, where borrower should be market borrower and account is sanctioned lender. However, inside _blockAccount function these params were added in wrong order, you can see that accountAddress, borrower, address(this) is passed, which means that WildcatSanctionsEscrow will have borrower where lender will be stored and account variable where will be borrower stored.

Because of that canReleaseEscrow function will work incorrectly as it will actually be checking that borrower is sanctioned instead of lender, which means that it will likely always return false. And in the end, borrower will be able to call releaseEscrow and receive all funds on his balance.

And in case if lender will be unsanctioned, then he will never get funds back, of course.

Exactly same behaviour currently inside WildcatMarketWithdrawals contract.

Impact

Borrower can take all funds of sanctioned lenders.

Tools Used

VsCode

Recommended Mitigation Steps

Provide params in correct order.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketBase.sol#L163-L187

Vulnerability details

Proof of Concept

In case if lender is sanctioned, then it's blocked and his market tokens are sent to the escrow. Pls, note that his market token still continue earning interests.

As result, lender can't interact with market anymore until he is not sanctioned anymore. Once it will be done, he will be able to get funds out of escrow.

It's possible that user will be locked forever. In this case his tokens will continue earning interests from borrower. As lender is sanctioned it's realy possible that borrower would like to not have any deal with such lender. So he will unregister such lender, but as i said lender's tokens will still accruing interests. This is because currently borrower doesn't have any ability to return back funds of sanctioned lender, so he don't use his funds anymore.

I believe that borrower should have functionality that allows him to repay all market tokens of sanctioned lender. In this case all assets will also go to the escrow. And borrower will not pay any interests for lender anymore and don't use his funds.

Impact

Borrower continue paying interests to the lender and uses his funds.

Tools Used

VsCode

Recommended Mitigation Steps

Escrow can have function that only borrower can trigger. This function will then ask market to burn escrow's market token and send them to the escrow. This will remove tokens from circulation then.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/WildcatMarketController.sol#L481

Vulnerability details

Proof of Concept

In case if borrower wants to change annual interests rate fro the market, then he can call WildcatMarketController.setAnnualInterestBips. In case if he is increasing rate then this is considered as good operation for lenders, so then nothing more is done and rate is updated.

But if he decreases rate, then protocol decided that it's time for user's to think about that situation and everyone who wants should be able to quit market. For such reason, market reserve ratio is set to 90% for 2 weeks. So lenders have 2 weeks to withdraw and then anyone can call resetReserveRatio function which will restore previous reserve ratio.

This is good thought, however it's possible that some markets will have bigger reserve ratio than 90%, for example 95% and really small interest rates. In this case, instead of increasing reserve ratio, protocol will decrease it, which will lead to the ability for borrower to borrow more funds without penalty.

Impact

Reserve ratio is decreased instead of increasing when annual interest rate is decreased.

Tools Used

VsCode

Recommended Mitigation Steps

Maybe reserve ratio for next 2 weeks should not be hardcoded, but should depend on previous one somehow.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarket.sol#L26-L29

Vulnerability details

Proof of Concept

Every time when any action with funds is executed on market, then _getUpdatedState function is called. The purpose of this function is to accrue interests and increase scaleFactor according to it. Usually borrower pays only annual interest rate + protocol fee rate, but when market is in delinquency, then user pays penalty.

So also after almost each action with funds, _writeState is called, which tracks if market isDelinquent. This is needed to know moments when penalty should be accrued or not.

Currently if user would like to repay funds, then he should just transfer them directly to the market. Such action will not call _getUpdatedState and not call _writeState, which means that in case if after the transfer market became not delinquent, this will not be noticed and borrower will continue accruing penalty, until _writeState is called.

Impact

User can pay more fees.

Tools Used

VsCode

Recommended Mitigation Steps

Create separate function that allows anyone to pay on behalf of borrower and then call _writeState and _getUpdatedState.

Assessed type

Error