A Sanctioned Address Can Not Only Repay Debt but Also Process Unpaid Withdrawal Batches Gracefully in WildcatMarketWithdrawals

[howlbot-integration[bot]]

Lines of code

https://github.com/0xastronatey/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarketWithdrawals.sol#L283

Vulnerability details

Description

The WildcatMarketWithdrawals contract in its current implementation, allows sanctioned addresses to repay debt and process unpaid withdrawal batches, violating the sponsor's invariant that sanctioned accounts should not modify the market state.

The repayAndProcessUnpaidWithdrawalBatches function lacks a sanction restriction check, contrary to other state-modifying functions like the executeWithdrawal() in the WildcatMarketWithdrawals contract.

Vulnerable code snippet from WildcatMarketWithdrawals.sol:

function repayAndProcessUnpaidWithdrawalBatches(
    uint256 repayAmount,
    uint256 maxBatches
) public nonReentrant sphereXGuardExternal {
    //@audit missing sanction check
    if (repayAmount > 0) {
        //@audit sanctioned addresses can repay
        asset.safeTransferFrom(msg.sender, address(this), repayAmount);
        emit_DebtRepaid(msg.sender, repayAmount);
    }

    MarketState memory state = _getUpdatedState();
    if (state.isClosed) revert_RepayToClosedMarket();

    if (repayAmount > 0) hooks.onRepay(repayAmount, state, _runtimeConstant(0x44));

    uint256 availableLiquidity = totalAssets() - 
        (state.normalizedUnclaimedWithdrawals + state.accruedProtocolFees);

    uint256 numBatches = MathUtils.min(maxBatches, _withdrawalData.unpaidBatches.length());

    uint256 i;
    while (i++ < numBatches && availableLiquidity > 0) {
        uint256 normalizedAmountPaid = _processUnpaidWithdrawalBatch(state, availableLiquidity);
        availableLiquidity = availableLiquidity.satSub(normalizedAmountPaid);
    }

    _writeState(state);
    //@audit sanctioned addresses can modify state
}

What matters is that sanctioned entities are not receiving or sending funds to the market, as this could put other lenders in a precarious legal situation. However, as seen in the implementation above, there is no restriction preventing sanctioned addresses from calling this function and sending funds into the market directly.

Impact

The lack of restrictions allows sanctioned addresses to, in fact, interact with the market, modify its state, and essentially circumvent financial regulations. This violates the key invariant stated in the brief: "Accounts which are flagged as sanctioned on Chainalysis should never be able to successfully modify the state of the market unless the borrower specifically overrides their sanctioned status in the sentinel (other than token approvals, or through their tokens being withdrawn & escrowed in nukeFromOrbit and executeWithdrawal)."

Proof of Concept

The issue can be demonstrated by a sanctioned address successfully calling the repayAndProcessUnpaidWithdrawalBatches(), which modifies the market state by reducing total debt and processing withdrawal batches. This action is currently allowed in the WildcatMarketWithdrawals contract!

Test output logs:

[PASS] test_repayAndProcessUnpaidWithdrawalBatches_SanctionedAddressCanRepay() (gas: 81568)
Logs:
  computeCreateAddress is deprecated. Please use vm.computeCreateAddress instead.
  prev market balance: 0.500000000000000000
  new market balance: 0.600000000000000000
  repaid amount: 0.100000000000000000

Create a new NewTest.t.sol file under ./test to run the POC code below with

forge test --match-path test/NewTest.t.sol -vvvv

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

import './BaseMarketTest.sol';

contract WildcatMarketSanctionTest is BaseMarketTest {
    using MathUtils for uint256;
    using SafeCastLib for uint256;
    using LibERC20 for address;

    address sanctionedAddress = address(0x3);
    MockSanctionsSentinel mockSentinel;

    function setUp() public override {
        super.setUp();
        mockSentinel = new MockSanctionsSentinel(address(archController));
        parameters.sentinel = address(mockSentinel);
        setUpContracts(false);

        mockSentinel.sanction(sanctionedAddress);

        _deposit(alice, 1e18);
        vm.prank(borrower);
        market.borrow(5e17);
    }

    function test_repayAndProcessUnpaidWithdrawalBatches_SanctionedAddressCanRepay() public {
        uint256 repayAmount = 1e17;
        uint256 maxBatches = 1; 
        asset.mint(sanctionedAddress, repayAmount);

        vm.startPrank(sanctionedAddress);
        asset.approve(address(market), repayAmount);

        uint256 prevMarketBalance = asset.balanceOf(address(market));

        // This should revert, but it doesn't
        market.repayAndProcessUnpaidWithdrawalBatches(repayAmount, maxBatches);

        uint256 newMarketBalance = asset.balanceOf(address(market));

        emit log_named_decimal_uint("prev market balance", prevMarketBalance, 18);
        emit log_named_decimal_uint("new market balance", newMarketBalance, 18);
        emit log_named_decimal_uint("repaid amount", repayAmount, 18);

        assertTrue(newMarketBalance > prevMarketBalance, "sanctioned address should not be able to repay and process unpaid withdrawal batches");
    }
}

Tools used

Foundry

Recommendation:

Implement a sanction check in the repayAndProcessUnpaidWithdrawalBatches() function.

function repayAndProcessUnpaidWithdrawalBatches( uint256 repayAmount, uint256 maxBatches ) public nonReentrant sphereXGuardExternal { // @audit add a sanction check to prevent sanctioned addresses from calling this function if (_isFlaggedByChainalysis(msg.sender)) { revert_repayAndProcessUnpaidWithdrawalBatchesWhileSanctioned(); }

// ..SNIP..
...

}

Assessed type

Access Control

[laurenceday]

https://github.com/code-423n4/2024-08-wildcat-findings/issues/68#issuecomment-2388332528 https://github.com/code-423n4/2024-08-wildcat-findings/issues/70#issuecomment-2363909534

No.

[d1ll0n]

I kind of respect the level of confidence it must take to report 3 separate highs for the same issue that clearly does not result in loss of funds or disable major protocol functionality in any way.

[c4-judge]

3docSec marked the issue as duplicate of #70

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b