Sanctioned Addresses Can Bypass Checks Due to Incorrect Assembly Code

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarketBase.sol#L254-L273 https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarketBase.sol#L769-L792

Vulnerability details

The WildcatMarketBase contract includes critical functions that rely on low-level assembly code to enforce compliance checks, particularly to prevent sanctioned addresses from interacting with the contract. Two primary functions involved are _isSanctioned() and _createEscrowForUnderlyingAsset(). These functions interact with the sentinel contract to verify if an address is sanctioned and to manage escrow creation for underlying assets.

_isSanctioned() Function

The _isSanctioned() function is intended to determine if a given account is sanctioned by calling the sentinel.isSanctioned(borrower, account) function. It uses inline assembly to perform a staticcall and then checks the returndatasize() to ensure that the call returned the expected 32 bytes.

The relevant portion of the code is:

function _isSanctioned(address account) internal view returns (bool result) {
    address _borrower = borrower;
    address _sentinel = address(sentinel);
    assembly {
        let freeMemoryPointer := mload(0x40)
        mstore(0, 0x06e74444) // Function selector for isSanctioned(address,address)
        mstore(0x20, _borrower)
        mstore(0x40, account)
        // Incorrect order of evaluation
        if iszero(
            and(
                eq(returndatasize(), 0x20), // Evaluated before staticcall
                staticcall(gas(), _sentinel, 0x1c, 0x44, 0, 0x20)
            )
        ) {
            returndatacopy(0, 0, returndatasize())
            revert(0, returndatasize())
        }
        result := mload(0)
        mstore(0x40, freeMemoryPointer)
    }
}

_createEscrowForUnderlyingAsset() Function

Similarly, the _createEscrowForUnderlyingAsset() function is designed to create an escrow for the underlying asset by calling sentinel.createEscrow(borrower, accountAddress, tokenAddress). It also uses inline assembly to perform a call and checks the returndatasize().

The code snippet is:

function _createEscrowForUnderlyingAsset(address accountAddress) internal returns (address escrow) {
    address tokenAddress = address(asset);
    address borrowerAddress = borrower;
    address sentinelAddress = address(sentinel);

    assembly {
        let freeMemoryPointer := mload(0x40)
        mstore(0, 0xa1054f6b) // Function selector for createEscrow(address,address,address)
        mstore(0x20, borrowerAddress)
        mstore(0x40, accountAddress)
        mstore(0x60, tokenAddress)
        // Incorrect order of evaluation
        if iszero(
            and(
                eq(returndatasize(), 0x20), // Evaluated before call
                call(gas(), sentinelAddress, 0, 0x1c, 0x64, 0, 0x20)
            )
        ) {
            returndatacopy(0, 0, returndatasize())
            revert(0, returndatasize())
        }
        escrow := mload(0)
        mstore(0x40, freeMemoryPointer)
        mstore(0x60, 0)
    }
}

The flaw lies in the use of the logical and operator within the if statement in both functions. In EVM assembly, the and operator evaluates both operands before performing the operation, and there is no guaranteed order of evaluation. Consequently, the check eq(returndatasize(), 0x20) is evaluated before the staticcall or call is executed.

This incorrect order leads to returndatasize() being zero (since no call has been made yet), causing the equality check to fail. As a result, the entire if condition may evaluate incorrectly, leading to one of two undesired outcomes:

1. False Negatives: The function might incorrectly determine that an address is not sanctioned because it does not properly process the return data from the sentinel contract.

2. Unexpected Reverts: The function might revert unexpectedly, even when interacting with non-sanctioned addresses, due to the premature check of returndatasize().

Impact

Sanctioned addresses can bypass compliance checks and interact with the contract, violating regulatory requirements and exposing the system to legal and reputational risks. Additionally, legitimate users may experience unexpected transaction failures, disrupting normal contract operations.

Proof of Concept

1. Scenario Setup:

   • The sentinel contract is responsible for identifying sanctioned addresses.
   • A sanctioned address, attacker, attempts to interact with the WildcatMarketBase contract.

2. Function Call:

   • The contract invokes _isSanctioned(attacker) to check if the attacker is sanctioned.

3. Execution Flow:

   • Inside _isSanctioned(), the assembly code attempts to perform a staticcall to sentinel.isSanctioned(borrower, attacker).
   • Due to the flawed assembly logic, eq(returndatasize(), 0x20) is evaluated before the staticcall occurs.
   • Since returndatasize() is zero at this point, the equality check fails.
   • The and operation does not short-circuit in EVM assembly; both operands are evaluated regardless.
   • The staticcall might not be executed properly, or its result is not correctly processed.

4. Outcome:

   • The function may incorrectly set result to false, indicating that the attacker is not sanctioned.
   • The attacker proceeds to interact with the contract without being blocked.

5. Alternate Outcome (Legitimate User):

   • A legitimate user attempts to interact with the contract.
   • Due to the premature returndatasize() check, the function may revert unexpectedly.
   • This causes a denial of service for legitimate users.

Tools Used

Manual review

Recommended Mitigation Steps

To resolve the issue, restructure the assembly code to ensure that the staticcall or call is executed before checking returndatasize(). Store the success status of the call in a variable and perform the checks afterward.

Assessed type

Other

[d1ll0n]

In EVM assembly, the and operator evaluates both operands before performing the operation, and there is no guaranteed order of evaluation. Consequently, the check eq(returndatasize(), 0x20) is evaluated before the staticcall or call is executed.

This is incorrect. All binary operators evaluate right to left in yul.

[3docSec]

Right to left seems the way to go indeed. A coded PoC would've saved the warden some precious time

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid