The Wildcat Protocol - ggg_ttt_hhh's results

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarket.sol#L142 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/libraries/FeeMath.sol#L159-L165

Vulnerability details

Impact

Perhaps the borrower intends to close the market, or the protocol compels the borrower to do so. In either scenario, the closeMarket function is invoked, setting annualInterestBips and reserveRatioBips to 0. And the necessary funds are made available in the market to fully cover all upcoming withdrawals from lenders. However, the delinquency fee remains unchanged. Therefore, if the market had already been in a delinquent state for a period exceeding the grace period before closing, the delinquency fee will still be applied in the future. This leads to an increase in the scaleFactor. As a result, the actual amount received by lenders will be greater than what they would have received at the time of market closure. Late lenders may not receive their assets because the borrower had already given up on this market, and the funds had already been consumed by former lenders.

Proof of Concept

Let's consider an example where delinquencyGracePeriod = 2000 and delinquencyFeeBips = 1000. Additionally, the market had accumulated 4000 in delinquent states before closing. In other words, the timeDelinquent is 4000. When the market closes, an adequate amount will be infused into the market, allowing it to recover from its delinquent state. However, during the subsequent 2000 units of time, the delinquency fee will continue to be applied, further increasing the scaleFactor.

Please add test below to test/market/WildcatMarket.t.sol and run test. The test will be failed.

    function test_closeMarket_AfterDelinquent() external asAccount(address(controller))
    {
        _deposit(alice, 1e18);
        _deposit(bob, 1e18);
        // Borrow 60% of market assets
        _borrow(12e17);
        assertEq(market.currentState().isDelinquent, false);
        // Withdraw 100% of deposits
        _requestWithdrawal(alice, 1e18);
        // The market becomes delinquent
        assertEq(market.currentState().isDelinquent, true);

        // delinquencyGracePeriod = 2000
        fastForward(4000);
        uint256 scaleFactor_1 = market.scaleFactor();
        asset.mint(address(market), 2e18);
        market.closeMarket();

        // The delinquency fee is applied
        fastForward(2000);
        uint256 scaleFactor_2 = market.scaleFactor();
        // The test will be failed
        assertEq(scaleFactor_1, scaleFactor_2);
    }

Tools Used

Recommended Mitigation Steps

Modify the FeeMath/updateScaleFactorAndFees function as follows:

- if (delinquencyFeeBips > 0) {
+ if (delinquencyFeeBips > 0 && !state.isClosed) {

The test will be passed.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarket.sol#L142 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L136-L139 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L119 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L77 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L243

Vulnerability details

Impact

The protocol provides the functionality to close a specific market. However, in practice, no one, including the borrower and the protocol, can call this function.

Proof of Concept

The closeMarket function is equipped with the onlyController modifier.

function closeMarket() external onlyController nonReentrant {

In this modifier, we verify whether the message sender is the controller.

modifier onlyController() {
  if (msg.sender != controller) revert NotController();
  _;
}

The controller's address is set within the parameters obtained from the controller that deployed this market.

MarketParameters memory parameters = IWildcatMarketController(msg.sender).getMarketParameters();
controller = parameters.controller;

function getMarketParameters() external view returns (MarketParameters memory parameters) {
   parameters.controller = address(this);
}

It's worth noting that the controller is implemented as a smart contract, not an externally owned account (EOA), and there is no function available to invoke the closeMarket function of the Market directly.

The same applies to the setMaxTotalSupply function.

Tools Used

Recommended Mitigation Steps

You can make one of the following changes:

Change to onlyBorrower Modifier: You can modify the onlyController modifier to the onlyBorrower modifier, which would allow only the borrower to invoke the closeMarket function.

OR

Create a Function in Controller Contract: You can create a specific function in the controller contract that can be called to invoke the closeMarket function, thereby enabling controlled access to this operation.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L95-L99 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L172-L176 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L166-L170

Vulnerability details

Impact

When blocks an account or sanctioned user attempts to execute a withdrawal, the protocol automatically generates an escrow on the user's behalf and transfers tokens to this escrow. However, we made an error in parameter order by reversing the positions of the borrower and account, resulting in improper functioning of the escrow. As a result, we will be checking the sanction state of the borrower instead of the account. And the funds will be sent to the borrower instead of the account.

Proof of Concept

Here is the actual createEscrow function:

function createEscrow(
    address borrower,
    address account,
    address asset
) public override returns (address escrowContract) {

And the meanings of borrower and account are self-explanatory. In the _blockAccount and executeWithdrawal function, we create an escrow as follows:

address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
   accountAddress,
   borrower,
   address(this)
);

Obviously, the order of borrower and account is reversed.

Tools Used

Recommended Mitigation Steps

In the _blockAccount and executeWithdrawal function, change the order of borrower and account.

address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
   borrower,
   accountAddress,
   address(this)
);

We can also fix the IWildcatSanctionsSentinel interface.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L468-L488

Vulnerability details

Impact

Each controller defines specific limits for interest rates, and all markets deployed through that controller should have interest rates that adhere to those limits. However, the setAnnualInterestBips function allows for the assignment of arbitrary annual interest rates to a market.

Furthermore, the readme file contains the following statement: Consider ways in which market interest rates can be manipulated to produce results that are outside of controller-specific limits

Proof of Concept

For testing purposes, please make a minor modification to the test/shared/TestConstants.sol as follows:

- uint16 constant MaximumAnnualInterestBips = 10_000;
+ uint16 constant MaximumAnnualInterestBips = 9_000;

Please add test below to test/WildcatMarketController.t.sol and run test. The test will be failed. In other words, it is possible to apply an annual interest rate to the market that exceeds the maximum limit.

    function test_setAnnualInterestBips_Exceeded() public {
        vm.prank(borrower);
        vm.expectRevert(AnnualInterestBipsOutOfBounds.selector);
        // The test will be failed
        controller.setAnnualInterestBips(address(market), MaximumAnnualInterestBips + 1);
    }

Tools Used

Recommended Mitigation Steps

Include the following check in the setAnnualInterestBips function:

     function setAnnualInterestBips(
        address market,
        uint16 annualInterestBips
    ) external virtual onlyBorrower onlyControlledMarket(market) {
        // If borrower is reducing the interest rate, increase the reserve
        // ratio for the next two weeks.
        + assertValueInRange(
        +     annualInterestBips,
        +     MinimumAnnualInterestBips,
        +     MaximumAnnualInterestBips,
        +     AnnualInterestBipsOutOfBounds.selector
        + );
     }

The test will be passed.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L484 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L95

Vulnerability details

Impact

When the withdrawalBatchDuration is set to 0, duplicate expiry values can be inserted into the unpaidBatches of _withdrawalData. Consequently, when any user of the protocol attempts to see the number of unpaid batches, the protocol may return duplicated values, such as [1, 2, 2]. This can have a negative impact on the reputation of the protocol.

Proof of Concept

Consider a scenario where withdrawalBatchDuration is set to 0. User A attempts to queue a withdrawal, but the market doesn't have sufficient funds to process it.

function queueWithdrawal(uint256 amount) external nonReentrant {
    if (state.pendingWithdrawalExpiry == 0) {
      state.pendingWithdrawalExpiry = uint32(block.timestamp + withdrawalBatchDuration);
      emit WithdrawalBatchCreated(state.pendingWithdrawalExpiry);
    }
}

In the same block, another User B, also tries to queue a withdrawal, triggering the execution of _processExpiredWithdrawalBatch (part of _getUpdatedState function).

function _processExpiredWithdrawalBatch(MarketState memory state) internal {
    if (batch.scaledAmountBurned < batch.scaledTotalAmount) {
      _withdrawalData.unpaidBatches.push(expiry);
    }
}

At this point, the expired batch is associated with User A, and its expiry matches the block's timestamp. Then, the expired batch for User B is updated, and its expiry also aligns with the block's timestamp because all these transactions occur within the same block. Subsequently, when any static call is made to the market, _processExpiredWithdrawalBatch is executed again, and the same expiry value is inserted into the unpaidBatches of _withdrawalData.

For testing purposes, please make a minor modification to the test/helpers/ExpectedStateTracker.sol as follows:

- withdrawalBatchDuration: DefaultWithdrawalBatchDuration,
+ withdrawalBatchDuration: 0,

Add test below to test/market/WildcatMarketBase.t.sol and will observe that the unpaidBatchExpires contains duplicate expiry values.

   function test_zero_WithdrawalBatchDuration() external {
        _deposit(alice, 1e18);
        _deposit(bob, 1e18);
        _borrow(12e17);
        _requestWithdrawal(alice, 1e18);
        _requestWithdrawal(bob, 1e18);
        market.updateState();
        uint32 expiry = uint32(block.timestamp);
        uint32[] memory unpaidBatchExpires = market.getUnpaidBatchExpiries();
        assertEq(unpaidBatchExpires[0], expiry);
        assertEq(unpaidBatchExpires[1], expiry);
    }

Tools Used

Recommended Mitigation Steps

Change _processExpiredWithdrawalBatch function as below:

function _processExpiredWithdrawalBatch(MarketState memory state) internal {
      if (batch.scaledAmountBurned < batch.scaledTotalAmount) {
            - _withdrawalData.unpaidBatches.push(expiry);
            
            + if (withdrawalBatchDuration > 0) {
            +     _withdrawalData.unpaidBatches.push(expiry);
            + } else {
            +    uint128 len = _withdrawalData.unpaidBatches.length();
            +    if (
            +        len == 0 ||
            +        _withdrawalData.unpaidBatches.at(len - 1) != expiry
            +    ) {
            +        _withdrawalData.unpaidBatches.push(expiry);
            +    }
            + }
        }
}

This adjustment will guarantee that unpaidBatchExpires contains only unique values and will not impact other functionalities.

Assessed type

Error