The Wildcat Protocol - 0xStalin's results

Lines of code

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

Vulnerability details

Impact

• Borrowers will never be able to close markets.

Proof of Concept

• The WildcatMarket::closeMarket() function has defined the onlyController modifier, which validates that the caller is the Controller that was registered when the Market was deployed, this means that only the Controller contract is allowed to call this function, but, in the WildcatMarketController contract there is not any single reference that calls the closeMarket() function.
  • As we can see in the below code, the onlyController modifier is validating the caller to be the Controller, otherwise, the call to the closeMarket() is reverted.

WildcatMarketBase contract

  modifier onlyController() {
    //@audit-info => If the caller is not the controller contract, the tx is reverted!
    if (msg.sender != controller) revert NotController();
    _;
  }

WildcatMarket contract

  //@audit-info => onlyController modifier enforces that the only allowed caller is the Controller contract
  function closeMarket() external onlyController nonReentrant {
    ...
    ...
    ...
  }

Tools Used

Manual Audit

Recommended Mitigation Steps

• There are two possible solutions to resolve this problem
  • The first option is to make sure to create a new function in the Controller contract and implement the required logic to validate that only valid borrowers can call valid markets they own, and if so, do the actual call to the closeMaker()
  • The second option could be to change the modifier in the closeMarket() function, instead of using the onlyController modifier, opt to use the onlyBorrower() modifier, which it validates the caller is the borrower, otherwise the tx is reverted

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

• When the original maxTotalSupply is reached, the market won't be able to receive more deposits because the maxTotalSupply can't be updated.

Proof of Concept

• The WildcatMarketConfig::setMaxTotalSupply() function is restricted to be called only by the controller, it has implemented the onlyController modifier, which enforces the caller to be the Controller contract. The problem is that in the WildcatMarketController contract there is not a single reference that calls the setMaxTotalSupply(), thus, the borrowers won't be able to update the original value that was set for the maxTotalSupply variable.
  • As we can see in the below code, the setMaxTotalSupply() is only callable by the controller.

WildcatMarketBase contract

  modifier onlyController() {
    //@audit-info => If caller is not the controller contract, the tx is reverted!
    if (msg.sender != controller) revert NotController();
    _;
  }

WildcatMarketConfig contract

  //@audit-info => onlyController modifier enforces that the only allowed caller is the Controller contract
  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);
  }

Tools Used

Manual Audit

Recommended Mitigation Steps

• Make sure to create a function in the Controller contract that runs the proper validations and will end up calling the setMaxTotalSupply(), in this way, the borrowers will be able to update the state.maxTotalSupply if it's required.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

• Lenders can escape the sanctioning of their account in any market.

Proof of Concept

• Before diving into the details of how the lenders can escape the sanctioning of their account.
  • First, let's analyze how a lender can be excised from a Market
    • When someone calls nukeFromOrbit within that market while flagged as sanctioned by the Chainanalysis oracle
    • When the lender invokes executeWithdrawal while flagged as sanctioned by the Chainalysis oracle
  • In either of the two options, the execution flow calls the Sentinel::isSanctioned() function to verify if the account(lender) is sanctioned by the borrower of the market
    • By analyzing the Sentinel::isSanctioned() function, it can be noted that the lender's account must have been sanctioned in the Oracle first before the account is finally sanction in a Market

WildcatSanctionsSentinel.sol

  function isSanctioned(address borrower, address account) public view override returns (bool) {
    //@audit-info => sanctionOverrides[borrower][account] must be false <==> sanction must not be overridden for this function to return true!
    //@audit-info => If sanctionOverrides[borrower][account] is set to true, this function will return false, as if the account would not be sanctioned

    //@audit-info => For this function to return true, the account's sanction should have not been overridden (it's set to false), and the account must have been sanctioned in the ChainalysisSanctionsList Oracle.
    return
      !sanctionOverrides[borrower][account] &&
      IChainalysisSanctionsList(chainalysisSanctionsList).isSanctioned(account);
  }

• Now, based on the previous explanation, we know that the lender's account needs to be sanctioned in the Chainalysis Oracle before the Sentinel::isSanctioned() function is called.

  • This opens up the doors for lenders who realize that their account has been sanctioned in the Chainalysis Oracle to move their MarketTokens to different accounts before the lender's account is fully blocked in the Market (you may be wondering what's the point of transferring tokens to accounts that have not been granted any role in the Market, I'll explain more about this in a sec, bear with me).
    • So, the lender transfers his MarketTokens to different accounts using the WildcatMarketToken::transfer() function, as a result, the lender's account that is sanction in the Chainalysis Oracle has no MarketTokens anymore, all those tokens have been moved to another accounts.
      • Now, at this point, anybody could call the nukeFromOrbit() to fully sanction the lender's account in a specific Market, either way, the Lender has already moved his tokens to other accounts.

• So, at this point, the lender's MarketTokens were distributed among different accounts of his own, such accounts have never interacted with the Market, so, their current role is the Null Role.

• Everything might look fine because the accounts where the tokens were sent have no permissions to interact with the Market, but there is a bug that allows lenders to gain the WithdrawOnly Role on any account they want without having the consent of the borrower

  • This problem is located in the WildcatMarketController::updateLenderAuthorization() function, the reason of this problem will be explained in the below code walkthrough:
    • In short, the Lender will be able to set the WithdrawOnly Role to any account he wishes, the reason is that any account that is not registered in the _authorizedLenders variable of the Controller will forward the value of _isAuthorized as false, and in the WildMarketConfig::updateAccountAuthorization() function, because the value of _isAuthorized is false, it will end up granting the WithdrawOnly Role.
      • This effectively allows any Lender to grant the WithdrawOnly Role to any account they want to.

WildcatMarketController.sol

  //@audit-info => Anybody can call this function and pass a lender and an array of markets where the changes will be applied!
  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();
      }
      //@audit-info => Forwards the value of the `lender` argument, and depending on the `lender` address is found in the _authorizedLenders EnumerableSet.AddressSet, will be forwarded a true or false accordingly
        //@audit => If the lender address is not found in the _authorizedLenders variable, it will forward a false to the Market::updateAccountAuthorization() function
      WildcatMarket(market).updateAccountAuthorization(lender, _authorizedLenders.contains(lender));
    }
  }

EnumerableSet.sol

function contains(AddressSet storage set, address value) internal view returns (bool) {
    //@audit-info => Calls the internal _contains()
    //@audit-info => If the given value is found it will return true, otherwise it will return false!
    return _contains(set._inner, bytes32(uint256(uint160(value))));
}

//@audit-info => The internal function will just return a true or false if the given value is in the set or not, but the tx won't be reverted!
/**
* @dev Returns true if the value is in the set. O(1).
*/
function _contains(Set storage set, bytes32 value) private view returns (bool) {
    return set._indexes[value] != 0;
}

WildcatMarketConfig.sol

  function updateAccountAuthorization(
    address _account,
    //@audit-info => For any account that is not registered in the `_authorizedLenders` of the Controller, this flag was set as false!
    bool _isAuthorized
  ) external onlyController nonReentrant {
    MarketState memory state = _getUpdatedState();
    //@audit-info => If the accountAddress is not registered in the storage, the approval role is set to Null
    //@audit-info => If the account has been blacklisted, tx will revert!
    Account memory account = _getAccount(_account);
    if (_isAuthorized) {
      account.approval = AuthRole.DepositAndWithdraw;
    
    //@audit => Any account not registered in the Controller will be assigned the WithdrawOnly role.
    } else {
      account.approval = AuthRole.WithdrawOnly;
    }
    _accounts[_account] = account;
    _writeState(state);
    emit AuthorizationStatusUpdated(_account, account.approval);
  }

• So, at this point, the Lender has been able to move their MarketTokens to different accounts and to grant the WithdrawOnly Role to all of the accounts he wishes to.
• Now they can decide to exit the Market by queuing and executing some withdrawal requests from the different accounts where the MarketTokens were moved, any of those accounts have now the WithdrawOnly Role and have a balance of MarketTokens, so, the Lender will be able to exit the market from any of those accounts.

Tools Used

Manual Audit

Recommended Mitigation Steps

• The mitigation for this problem is very straight-forward, limiting the access to which entities can call the WildcatMarketController::updateLenderAuthorization() function, either only allow the Borrower to call it, or create a type of withelist of valid actors who are capable of updating the lender's authorization on the Markets, in this way, the Lenders won't be capable of granting the WithdrawOnly Role to any account they want to, thus, they won't be able even to attempt to escape the sanctions.

WildcatMarketController.sol

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

modifier onlyAuthorizedEntities() {
    require(msg.sender == <authorizedEntities>, "you are not allowed sir");
    _;
}

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/WildcatSanctionsEscrow.sol#L33-L41 https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketToken.sol#L15-L19 https://github.com/code-423n4/2023-10-wildcat/blob/main/src/libraries/FeeMath.sol#L50

Vulnerability details

Impact

• If the sanctions on the lender are lifted, the Borrower will end up paying interest on all the time that the sanction on the Lender was active, and the lender will gain the benefit as any other Lender who was not sanctioned.
• The protocolFees will charge fees on all the locked MarketTokens in the escrows, so, the protocolFees will compound overtime considering MarketTokens that are sanctioned and not usable by anyone.

Proof of Concept

• As for the Borrower that will end up paying interests on all the time that the sanction on the Lender was active:
  • The existing logic of the escrow when releasing the escrow is using the balanceOf() function of the MarketToken contract, which returns the normalized balance of account with interest.
  • The logic is to compute how many asset's market tokens represent the whole markettoken's balance owned by the escrow, and then that amount is forwarded to the transfer() of the MarketToken contract, which will compute the scaleBalance for that amount, which it will effectively do the transfer of all the MarketTokens that were locked in the escrow, and then, when the lender receives the MarketTokens, it will be able to withdraw them and receive all the interests that were accrued during the whole time the lender was sanctioned!

WildcatSanctionsEscrow.sol

function releaseEscrow() public override {
    if (!canReleaseEscrow()) revert CanNotReleaseEscrow();

    //@audit-info => Will call the balanceOf() of the asset contract!
    uint256 amount = balance();

    //@audit-info => Transfers the tokens to the account out of this escrow!
    //@audit-info => When the asset is a MarketToken, the `amount` has already been accounted with all the interests that were accrued during all the time the Lender's account was sanctioned!
    IERC20(asset).transfer(account, amount);

    emit EscrowReleased(account, asset, amount);
}

function balance() public view override returns (uint256) {
    //@audit-info => When the escrow is holding MarketContracts, will call the balanceOf() of the WildcatMarketToken contract!
    return IERC20(asset).balanceOf(address(this));
}

WildcatMarketToken.sol

function balanceOf(address account) public view virtual nonReentrantView returns (uint256) {
    (MarketState memory state, , ) = _calculateCurrentState();
    //@audit-info => By normalizing the amount will account for all the interests that were generated during all the time the Lender's account was sanctioned!
    return state.normalizeAmount(_accounts[account].scaledBalance);
}

function _transfer(address from, address to, uint256 amount) internal virtual {
    MarketState memory state = _getUpdatedState();
    uint104 scaledAmount = state.scaleAmount(amount).toUint104();

    if (scaledAmount == 0) {
      revert NullTransferAmount();
    }

    //@audit-info => Will transfer all the MarketTokens that were locked in the Escrow to the Account
    //@audit-info => Then, the Account will be able to withdraw those MarketTokens for the asset's market and will receive all the interest that was accrued during the sanctioned period

    Account memory fromAccount = _getAccount(from);
    fromAccount.scaledBalance -= scaledAmount;
    _accounts[from] = fromAccount;

    Account memory toAccount = _getAccount(to);
    toAccount.scaledBalance += scaledAmount;
    _accounts[to] = toAccount;

    _writeState(state);
    emit Transfer(from, to, amount);
  }

• As for the protocolFees charging fees on the MarketTokens that are locked in the escrow:
  • When the MarketTokens are transfered to the escrow to be locked during all the time that the Lender is sanctioned, the totalSupply of market tokens is not reduced, it's left untouched, this will cause that each time that the protocolFees are computed, all the market tokens of all the sanction lenders that are locked in escrows are also considered when computing the protocol fees.

FeeMath.sol

  function applyProtocolFee(
    MarketState memory state,
    uint256 baseInterestRay,
    uint256 protocolFeeBips
  ) internal pure returns (uint256 protocolFee) {
    // Protocol fee is charged in addition to the interest paid to lenders.
    uint256 protocolFeeRay = protocolFeeBips.bipMul(baseInterestRay);

    //@audit-info => Here, the scaledTotalSupply of the MarketTokens is the base to compute the protocolFee
    //@audit-info => All the MarketTokens locked in escrows will also be accounted as part of the protocolFee
    protocolFee = uint256(state.scaledTotalSupply).rayMul(
      uint256(state.scaleFactor).rayMul(protocolFeeRay)
    );
    state.accruedProtocolFees = (state.accruedProtocolFees + protocolFee).toUint128();
  }

• The existing logic of how the sanctions are managed will cause the borrower to end up paying interests all the time that the sanctions are active, which doesn't sound very fair for the borrower nor for the rest of the Lenders if a Lender account is sanctioned it should not receive the same benefits as the rest of accounts that are not sanctioned.

Tools Used

Manual Audit

Recommended Mitigation Steps

• My recommendation to fix this problem is to tweak a little bit the existing logic when sanctioning lenders, instead of locking their MarketTokens, opt to lock the equivalent of the assets that the Lender's MarketToken represents at the time of the sanction (as if the lender would be queing and executing a withdrawl), this will correctly lock the equivalent of assets in the Escrow given the current scaleFactor at the time of the sanction, in this way, if the borrower ever lifts up the sanction on the Lender, the locked assets were not accruing interest, because they are not MarketTokens, they are normal tokens, so, at that time, the Lender can go ahead an get back the assets (not MarketTokens) that were locked in the Escrow, and the Borrower won't pay interests for all the time that the Lender was sanction (because the locked assets were payed based on the scaleFactor at the time of the sanction), and if the Borrower grants the permission to the Lender to become a depositor again, the Lender can deposit the received assets in the Market, and it will receive an amount of MarketTokens given the scaleFactor at that point in time.
  • In this way, the Borrower won't end up paying interests to the sanctioned Lender for all the time that the sanctions were active.
  • Also, by locking the asset tokens instead of the MarketTokens, the MarketTokens of the Lender will be burnt, so, the totalSupply will be reduced, which will end up fixing the problem of the protocolFees charging fees on all those locked MarketTokens, now, the locked assets are not MarketTokens, as a fact, those MarketTokens were burnt, so, the totalSupply was reduced, thus, the fees won't be charged on the assets locked in the escrow.

Assessed type

Context

Judge has assessed an item in Issue #571 as 2 risk. The relevant finding follows:

[L-01] Internal accounting won’t work for rebase & elastic tokens

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketToken.sol#L59-L82

Vulnerability details

Impact

• As time passes (and the scaleFactor) grows, the real amount of MarketTokens that the spenders will be able to spend on behalf of the approver will continuously decrease.

Proof of Concept

• The current implementation of the approvals functionality is such that the approver grants X amount of MarketTokens to the spender to be spent on his behalf, the problem is that the accounting of the MarketTokens is being handled in ray units (scaledAmounts), not in normal units, as opposed to the accounting of the approvals mechanism which is handled in normal units, this difference will cause that as the time passes (and the scaleFactor) grows, the real amount of MarketTokens that the spenders will be allowed to spend, it will continuously decrease as the scaleFactor grows.

• In example, Alice approves Bob to spend 10 MarketTokens on her behalf. Time passes, and Bob wants to spend all his allowance, now the scaleFactor has grown from 1.5 to 2, as a result, because of the existing implementation, Bob's allowance will be reduced to 0, but the real amount of MarketTokens that were actually transferred will be less than 10, the reason is that in the _transfer() function, the input amount (10), it will be scaled considering the value of the scaleFactor, this will cause that the computed value of the scaledAmount will be less than 10, thus, Bob was able to transfer less than what he was allowed to.

WildcatMarketToken.sol

function _approve(address approver, address spender, uint256 amount) internal virtual {
    //@audit-info => Allowances are not saved in scaledUp units
    //@audit-info => Spender receives as allowance the exact input amount, regardless of the value of the scaleFactor!
    allowance[approver][spender] = amount;
    emit Approval(approver, spender, amount);
}

  function transferFrom(
    address from,
    address to,
    uint256 amount
  ) external virtual nonReentrant returns (bool) {
    uint256 allowed = allowance[from][msg.sender];

    // Saves gas for unlimited approvals.
    if (allowed != type(uint256).max) {
      //@audit-info => The inputted amount is deducted from the allowance
      //@audit-info => i.e. Bob wants to transfer 10 MarketTokens from Alice, the current implementation will reduce the allowance of Bob by 10, regardless of how many MartketTokens are actually transferred in the below _transfer() function
      uint256 newAllowance = allowed - amount;
      _approve(from, msg.sender, newAllowance);
    }

    _transfer(from, to, amount);

    return true;
  }

  function _transfer(address from, address to, uint256 amount) internal virtual {
    MarketState memory state = _getUpdatedState();
    //@audit-info => The inputted amount is scaled, this is considering the current value of the scaleFactor, which as its value grows, the scaledAmount will continuously decrease.
    uint104 scaledAmount = state.scaleAmount(amount).toUint104();

    if (scaledAmount == 0) {
      revert NullTransferAmount();
    }

    //@audit-info => Does the actual transfer of MarketTokens from one account to the other
    //@audit-issue => The real amount of MarketTokens will be less than the inputted amount, the reason is, as the scaleFactor grows, the scaledAmount will always be lower and lower
        //@audit-info => The bigger scaleFactor, the lower the computed scale amount!
    Account memory fromAccount = _getAccount(from);
    fromAccount.scaledBalance -= scaledAmount;
    _accounts[from] = fromAccount;

    Account memory toAccount = _getAccount(to);
    toAccount.scaledBalance += scaledAmount;
    _accounts[to] = toAccount;

    //@audit-info => As a result, if Bob was allowed to spend 10 MarketTokens, the real amount of MarketTokens that were transferred will be lower and lower as time passes and scaleFactor grows

    _writeState(state);
    emit Transfer(from, to, amount);
  }

Tools Used

Manual Audit

Recommended Mitigation Steps

• I think the easiest solution is to also scale up the accounting of the allowance functionality by using the state.scaleAmount() function
• In this way, the spender will actually be able to spend the total amount of MarketTokens that it was approved to spend, regardless of at what point in time decides to spend that allowance, and no matter how much the scaleFactor grows
• Even though the scaleFactor grows as time passes, the spenders will preserve their approved MarketToken's balances
  • By implementing this suggestion, the approver can be assured that their spenders will be able to spend the exact amount of MarketTokens that they were allowed, no matter how much the scaleFactor grows

Assessed type

Context