The Wildcat Protocol - cartlex_'s results

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

L-01 Controller can’t call setMaxTotalSupply due to lack of function to do it.

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L137-L188 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L166-L170 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/WildcatSanctionsEscrow.sol#L33-L41 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L39-L43 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L173-L175

Vulnerability details

Impact

Lender unexpectedly can transfer funds to the borrower instead of itself and lost them.

Proof of Concept

When lender want to withdraw underlying asset from the market contract executeWithdrawal() function can be used.

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

However, if lender have been added to the sanction list, amount of funds that lender want to withdraw transfers to the escrow contract, which is creating inside executeWithdrawal() using createEscrow() function.

    ...
if (IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
    _blockAccount(state, accountAddress);
    address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
        accountAddress,
        borrower,
        address(asset)
    );
        asset.safeTransfer(escrow, normalizedAmountWithdrawn);
        emit SanctionedAccountWithdrawalSentToEscrow(
            accountAddress,
            escrow,
            expiry,
            normalizedAmountWithdrawn
        );
    } else {
    asset.safeTransfer(accountAddress, normalizedAmountWithdrawn);
}
    ...

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

createEscrow() takes as arguments borrower, account and asset and has the following order.

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

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L95-L99

You can see that the order of arguments is different from executeWithdrawal().

As a consequence we have the next situation, that inside WildcatSanctionsEscrow.sol contract borrower == account, account == borrower, asset == address(market).

It's possible to release funds that was blocked on WildcatSanctionsEscrow.sol contract using releaseEscrow() function.

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

    uint256 amount = balance();

    IERC20(asset).transfer(account, amount);

    emit EscrowReleased(account, asset, amount);
}

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsEscrow.sol#L33-L41

Inside releaseEscrow() the canReleaseEscrow() function must return true so that the user can return funds back. In turn, the function canReleaseEscrow() checks wheter the account is sanctioned using isSanctioned() function.

function isSanctioned(address borrower, address account) public view override returns (bool) {
    return !sanctionOverrides[borrower][account] &&
    IChainalysisSanctionsList(chainalysisSanctionsList).isSanctioned(account);
}

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L39-L43

As I wrote earlier to complete the condition in releaseEscrow() we must have false as return value of isSanctioned() function, in order to fulfill this condition, it is sufficient that the lender is not sanctioned or overrided by the borrower. Since account == borrower and vice versa the isSanctioned() function returns false.

Since that, account can use releaseEscrow() function, but funds will be transferred to the borrower, since borrower == account.

Here is the link to gist with Proof of concept which is caused by this flaw.

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.20;

import "../src/interfaces/IMarketEventsAndErrors.sol";
import "./BaseMarketTest.sol";

interface ISanction {
    function addToSanctionsList(address[] memory newSanctions) external;
    function sanction(address account) external;
    function unsanction(address account) external;
}

interface IEscrow {
  function releaseEscrow() external;
  function balance() external view returns (uint256);
}

/**
 * To run the test:
 * 1. Create a <folder> where you can place initial files.
 * 2. Copy repository using `git clone https://github.com/code-423n4/2023-10-wildcat.git <folder>`.
 * 3. Copy `WildcatMarketConfig.high-02.t.sol` file from gist into foundry `test` folder.
 * 4. Use `forge test --mp test/WildcatMarketConfig.high-02.t.sol -vvvvv`.
 */

contract WildcatMarketConfigTest02 is BaseMarketTest {

    address public hole = makeAddr("hole"); // random address to burn token from Alice.

    function test_LenderCanUnblockItselfUseReleaseEscrow() external {
        vm.startPrank(alice);
        uint256 aliceStartBalance = asset.balanceOf(alice);
        uint256 aliceDepositAmount = 50_000e18;
        uint256 aliceWithdrawAmount = 35_000e18;

        // Preparation: Burn initial amount of tokens that Alice have, remain only 50_000 for better visualization in the test.
        asset.transfer(hole, aliceStartBalance);
        uint256 aliceStartBalanceAfterBurning = asset.balanceOf(alice);
        assertEq(aliceStartBalanceAfterBurning, 0);
        vm.stopPrank();

        _deposit(alice, aliceDepositAmount);
        _requestWithdrawal(alice, aliceWithdrawAmount);

        uint256 expiry = block.timestamp + parameters.withdrawalBatchDuration;
        fastForward(parameters.withdrawalBatchDuration);
        MarketState memory state = pendingState();
        updateState(state);

        // Block Alice address via chainalysis.
        address chainalysis = sanctionsSentinel.chainalysisSanctionsList();
        ISanction(chainalysis).sanction(alice);
        
        // Alice use `overrideSanction()` function with borrower's address.
        vm.startPrank(alice);
        market.executeWithdrawal(alice, uint32(expiry));

        sanctionsSentinel.overrideSanction(borrower);

        address escrowContractAddress = sanctionsSentinel.getEscrowAddress(alice, borrower, address(asset));
        bool isAliceSanctionedAfter = sanctionsSentinel.isSanctioned(alice, borrower);
        assertEq(isAliceSanctionedAfter, false);

        uint256 assetAliceBalanceBeforeEscrowing = asset.balanceOf(alice);
        uint256 assetBorrowerBalanceBeforeEscrowing = asset.balanceOf(borrower);
        
        assertEq(asset.balanceOf(borrower), 0);

        emit log_named_decimal_uint("Alice's balance of asset before `releaseEscrow()` function was used", assetAliceBalanceBeforeEscrowing, 18);
        emit log_named_decimal_uint("Borrower's balance of asset before `releaseEscrow()` function was used", assetBorrowerBalanceBeforeEscrowing, 18);
        console.log(StdStyle.magenta("==================================================================================================="));

        // Alice can use `releaseEscrow()` function to get her funds back...
        IEscrow(escrowContractAddress).releaseEscrow();

        // ...but funds were transferred to Borrower's address instead.
        uint256 assetAliceBalanceAfterEscrowing = asset.balanceOf(alice);
        assertEq(assetAliceBalanceAfterEscrowing, 0);

        uint256 assetBorrowerBalanceAfterEscrowing = asset.balanceOf(borrower);
        assertEq(assetBorrowerBalanceAfterEscrowing, aliceWithdrawAmount);
        
        emit log_named_decimal_uint("Alice's balance of asset after `releaseEscrow()` function was used", assetAliceBalanceAfterEscrowing, 18);
        emit log_named_decimal_uint("Borrowers's balance of asset after `releaseEscrow()` function was used", assetBorrowerBalanceAfterEscrowing, 18);
        console.log(StdStyle.magenta("==================================================================================================="));
        console.log(StdStyle.magenta("Alice used `releaseEscrow()` function, but funds were transferred to the borrower"));

        vm.stopPrank();
    }
}

Same flaw can be found in _blockAccount() function. When it calling createEscrow() function internally, the next arguments orderring is used:

address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
    accountAddress, // @audit invalid order of arguments
    borrower,
    address(this)
);

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

Tools Used

Manual review, foundry.

Recommended Mitigation Steps

Consider to change the order of input arguments in executeWithdrawal() and _blockAccount() functions when createEscrow() function is used.

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

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

Assessed type

Error