The Wildcat Protocol - Aymen0909's results

Lines of code

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

Vulnerability details

Impact

The issue arises because the market state is continually updated when lenders queue their withdrawals using queueWithdrawal, even if the market is closed. In this scenario, there is a case where the condition state.liquidityRequired() > totalAssets() becomes true, making the market delinquent. This, in turn, leads to the accrual of delinquency fees, which increases the scaleFactor.

As a consequence, even though the market is closed interest is still being accrued and as borrower won't pay for it (has no incentive to do so as he can't borrow anymore when market is closed) the total assets held in the market contract at the time of closure become insufficient to repay all the lenders.

Proof of Concept

When a market is closed in WildcatMarket.closeMarket, the function makes sure that the total amount of assets held in the contract is sufficient to repay all the lenders, for doing so the following condition must be satisfied totalAssets() == state.totalDebts() where the totals amount of debts is given as :

function totalDebts(MarketState memory state) internal pure returns (uint256) {
    return
      state.normalizeAmount(state.scaledTotalSupply) +
      state.normalizedUnclaimedWithdrawals +
      state.accruedProtocolFees;
}

Thus the totalDebts function assumes that the scaleFactor value will not increase anymore which is why it makes the calculation state.normalizeAmount(state.scaledTotalSupply) to get the current lended funds value.

Now when lenders want to withdraw their funds from the closed market they will call queueWithdrawal below :

function queueWithdrawal(uint256 amount) external nonReentrant {
    MarketState memory state = _getUpdatedState();

    // Cache account data and revert if not authorized to withdraw.
    Account memory account = _getAccountWithRole(
        msg.sender,
        AuthRole.WithdrawOnly
    );

    uint104 scaledAmount = state.scaleAmount(amount).toUint104();
    if (scaledAmount == 0) {
        revert NullBurnAmount();
    }

    // Reduce caller's balance and emit transfer event.
    account.scaledBalance -= scaledAmount;
    _accounts[msg.sender] = account;
    emit Transfer(msg.sender, address(this), amount);

    // If there is no pending withdrawal batch, create a new one.
    if (state.pendingWithdrawalExpiry == 0) {
        state.pendingWithdrawalExpiry = uint32(
            block.timestamp + withdrawalBatchDuration
        );
        emit WithdrawalBatchCreated(state.pendingWithdrawalExpiry);
    }
    // Cache batch expiry on the stack for gas savings.
    uint32 expiry = state.pendingWithdrawalExpiry;

    WithdrawalBatch memory batch = _withdrawalData.batches[expiry];

    // Add scaled withdrawal amount to account withdrawal status, withdrawal batch and market state.
    _withdrawalData
    .accountStatuses[expiry][msg.sender].scaledAmount += scaledAmount;
    batch.scaledTotalAmount += scaledAmount;
    state.scaledPendingWithdrawals += scaledAmount;

    emit WithdrawalQueued(expiry, msg.sender, scaledAmount);

    // Burn as much of the withdrawal batch as possible with available liquidity.
    uint256 availableLiquidity = batch.availableLiquidityForPendingBatch(
        state,
        totalAssets()
    );
    if (availableLiquidity > 0) {
        _applyWithdrawalBatchPayment(
            batch,
            state,
            expiry,
            availableLiquidity
        );
    }

    // Update stored batch data
    _withdrawalData.batches[expiry] = batch;

    // Update stored state
    _writeState(state);
}

When the first lender calls queueWithdrawal function it calculates scaledAmount being withdrawn using the scaleFactor saved at closure and then it will increments state.scaledPendingWithdrawals by the same amount (scaledAmount).

At the end of the call the function will invoke _writeState functions below :

function _writeState(MarketState memory state) internal {
    bool isDelinquent = state.liquidityRequired() > totalAssets();
    state.isDelinquent = isDelinquent;
    _state = state;
    emit StateUpdated(state.scaleFactor, isDelinquent);
}

To verify if a market is delinquent the following check is done state.liquidityRequired() > totalAssets(), where the liquidity required value is given by :

function liquidityRequired(
MarketState memory state
) internal pure returns (uint256 _liquidityRequired) {
    uint256 scaledWithdrawals = state.scaledPendingWithdrawals;
    uint256 scaledRequiredReserves = (state.scaledTotalSupply - scaledWithdrawals).bipMul(
      state.reserveRatioBips
    ) + scaledWithdrawals;
    return
      state.normalizeAmount(scaledRequiredReserves) +
      state.accruedProtocolFees +
      state.normalizedUnclaimedWithdrawals;
}

Because when the market was close we set state.reserveRatioBips == 0 the returned _liquidityRequired will be :

state.normalizeAmount(state.scaledPendingWithdrawals) +
state.accruedProtocolFees +
state.normalizedUnclaimedWithdrawals

This is the same formula used to calculate the total debts when closing the market except now we have state.scaledPendingWithdrawals at the place state.scaledTotalSupply, and because we know that usually we have (unless all the lenders decides to withdraw at the same time which is unlikely) :

state.scaledTotalSupply >= state.scaledPendingWithdrawals

The returned liquidity required value will be less than totalAssets() value and thus isDelinquent will be true which will update the global market state to delinquent that is state.isDelinquent = isDelinquent = true.

Because the market is closed the borrower has no incentive to make market delinquency state false again (by sending asset funds to the market), the market will remain delinquent forever which will have an impact on all the lenders that will withdraw afterwards (because scaleFactor will keep increasing) as explained in what follows.

When the others lenders make a call to queueWithdrawal and it invokes _getUpdatedState() the following code will be executed :

if (block.timestamp != state.lastInterestAccruedTimestamp) {
    (
        uint256 baseInterestRay,
        uint256 delinquencyFeeRay,
        uint256 protocolFee
    ) = state.updateScaleFactorAndFees(
            protocolFeeBips,
            delinquencyFeeBips,
            delinquencyGracePeriod,
            block.timestamp
        );
    emit ScaleFactorUpdated(
        state.scaleFactor,
        baseInterestRay,
        delinquencyFeeRay,
        protocolFee
    );
}

The updateScaleFactorAndFees function is called and as state.annualInterestBips is equal to 0 (set at closure) no base interest or protocol fee are accrued but the delinquencyFeeRay won't be zero as market is delinquent and thus the scaleFactor will be increased :

uint256 prevScaleFactor = state.scaleFactor;
//@audit baseInterestRay == 0 && delinquencyFeeRay != 0
uint256 scaleFactorDelta = prevScaleFactor.rayMul(baseInterestRay + delinquencyFeeRay);
state.scaleFactor = (prevScaleFactor + scaleFactorDelta).toUint112();

This means that even if the market is closed interest is still being accrued with the expectation that the borrower will cover that, which is not the case as the market is closed.

Now when the scaledAmount is calculated in queueWithdrawal it'll be using the new scaleFactor value and not the one saved at market closure, because this value is larger than the previous one the lender can ask for a bigger amount to withdraw when giving input amount to queueWithdrawal call.

At each lender call to queueWithdrawal the scaleFactor will be increased allowing the lenders to ask for a larger amount to be withdrawn from the market contract than what was intended when the market was closed, but we also know that the total amount of asset will no increase again as it was fixed when the market was closed and the borrower won't pay interest anymore.

Thus when all lenders start withdrawing, the early callers will be able to get more funds and profit from the increase in interest after the market closure but the late withdrawers won't get the amount they were intended to get as their portion of the total asset was withdrawn by other lenders (early withdrawers after market closure).

In summary, when a market is closed the total amount of asset held in the contract is fixed (constant) but the interest will still be accrued due to the delinquency fees still being collected (which will increase the scaleFactor value), as a consequence, some lenders (early withdrawers) will be able to withdraw more funds (for their scaled amount) while others lenders (late withdrawers) will receive a very small amount not the one intended for them (if the scaleFactor remained the same as when market was closed) or wan't be able to withdraw at all if all the market asset would have been already withdrawn.

Tools Used

Manual review

Recommended Mitigation Steps

To avoid this issue i recommend to make sure that the delinquency fees are not accrued when a market is closed, the simplest way to do so is to check if a market is closed in updateScaleFactorAndFees function before calculating delinquencyFeeRay

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The functions WildcatMarket.closeMarket and WildcatMarketConfig.setMaxTotalSupply both have the onlyController modifier, restricting them to be called exclusively by the market controller contract. However, the controller contract lacks any functions that allow calls to these specific functions. Consequently, no one can invoke these functions.

This situation poses a significant challenge for market management since there's no means to modify the maximum supply, and, more critically, it's impossible to ever close the market.

Proof of Concept

Both WildcatMarket.closeMarket and WildcatMarketConfig.setMaxTotalSupply functions have the onlyController modifier :

function closeMarket() external onlyController nonReentrant

function setMaxTotalSupply(uint256 _maxTotalSupply) external onlyController nonReentrant

And thus they can only be called by the market controller contract, but if you take a look at all the functions present in the WildcatMarketController contract we can easily see that none of them make a call to any of the two functions and thus the contract can never call them.

Normally we should find external functions that contain the following calls :

WildcatMarket(market).closeMarket()

WildcatMarket(market).setMaxTotalSupply(_maxTotalSupply)

As it is the case when changing the annual interest with setAnnualInterestBips for example.

This problem implies that the initial maximum total supply set during the market's creation cannot be modified, and the market cannot be closed by any party. These limitations pose significant constraints on the protocol's flexibility and borrowers' ability to interact with the market.

Tools Used

Manual review

Recommended Mitigation Steps

Add external functions in WildcatMarketController contract to allow the contract to make calls to both WildcatMarket.closeMarket and WildcatMarketConfig.setMaxTotalSupply functions.

Assessed type

Context

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/market/WildcatMarketWithdrawals.sol#L166-L170

Vulnerability details

Impact

When a lender gets sanctioned, an escrow is established to secure their funds. They should regain access to their funds once the sanctions are lifted. However, a critical issue arises when the createEscrow function is invoked within both the WildcatMarketBase._blockAccount and WildcatMarketWithdrawals.executeWithdrawal functions, in these instances, incorrect arguments are supplied to the function, leading to a situation where the lender is unable to withdraw their funds from the escrow, leaving the funds indefinitely trapped.

Proof of Concept

The createEscrow function from the WildcatSanctionsSentinel contract expects the following inputs :

function createEscrow(
    address borrower,
    address account,
    address asset
)

Where :

• borrower is the borrower account of the market creating the escrow.

• account is the lender who is currently sanctionned.

• asset the market asset (ERC20) which should be kept in the escrow contract.

Now when the first instance of this issue occurs in WildcatMarketBase._blockAccount below :

function _blockAccount(
    MarketState memory state,
    address accountAddress
) internal {
    Account memory account = _accounts[accountAddress];
    if (account.approval != AuthRole.Blocked) {
        uint104 scaledBalance = account.scaledBalance;
        account.approval = AuthRole.Blocked;
        emit AuthorizationStatusUpdated(accountAddress, AuthRole.Blocked);

        if (scaledBalance > 0) {
            account.scaledBalance = 0;
            //@audit wrong argument given to createEscrow
            //@audit should be (borrower,accountAddress,address(asset))
            address escrow = IWildcatSanctionsSentinel(sentinel)
                .createEscrow(accountAddress, borrower, address(this));
            emit Transfer(
                accountAddress,
                escrow,
                state.normalizeAmount(scaledBalance)
            );
            _accounts[escrow].scaledBalance += scaledBalance;
            emit SanctionedAccountAssetsSentToEscrow(
                accountAddress,
                escrow,
                state.normalizeAmount(scaledBalance)
            );
        }
        _accounts[accountAddress] = account;
    }
}

We can clearly see that the createEscrow function doesn't gets the correct input as both lender and borrower addresses are inverted (accountAddress correspond to the lender and should be the second input), in addition address(this) (the market address) is given as address for the ERC20 asset which is wrong as it's supposed to be address(asset).

The second instance occurs in WildcatMarketWithdrawals.executeWithdrawal :

function executeWithdrawal(
    address accountAddress,
    uint32 expiry
) external nonReentrant returns (uint256) {
    ...

    if (
        IWildcatSanctionsSentinel(sentinel).isSanctioned(
            borrower,
            accountAddress
        )
    ) {
        _blockAccount(state, accountAddress);
        //@audit wrong argument given to createEscrow
        //@audit should be (borrower,accountAddress,address(asset))
        address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
            accountAddress,
            borrower,
            address(asset)
        );
        asset.safeTransfer(escrow, normalizedAmountWithdrawn);
        emit SanctionedAccountWithdrawalSentToEscrow(
            accountAddress,
            escrow,
            expiry,
            normalizedAmountWithdrawn
        );
    } else {
        asset.safeTransfer(accountAddress, normalizedAmountWithdrawn);
    }

    ...
}

We see here also that both lender and borrower addresses are inverted again because accountAddress correspond to the lender address which should be the second input and borrower should be the first input.

Because of this errors we will have the following negative impacts on the lender :

• In the first instance, since the wrong asset address is provided, the lender will be unable to withdraw their funds from the escrow. The asset stored in the escrow will not correspond to the correct ERC20 token held in the escrow.

• In the second instance, only the borrower will have the capability to withdraw the funds from the escrow, as the borrower's address is used instead of the lender's.

In both cases, unfortunately, the lender will never be able to recover their funds.

Tools Used

Manual review

Recommended Mitigation Steps

To avoid both these issues the correct arguments must be given to the createEscrow function when invoked within both the WildcatMarketBase._blockAccount and WildcatMarketWithdrawals.executeWithdrawal functions.

Assessed type

Context