howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarket.sol#L202

Vulnerability details

Description

The WildcatMarket in its current implementation, contract allows sanctioned addresses to repay debt, violating the sponsor's invariant that sanctioned accounts should not modify the market state. The repay() and repayOutstandingDebt() functions lacks a sanction check, contrary to other state-modifying functions like borrow().

Let's take a look at the repay() and repayOutstandingDebt() implementations from WildcatMarket.sol:

  function repay(uint256 amount) external nonReentrant sphereXGuardExternal {
    //@audit missing sanction check for sender
    if (amount == 0) revert_NullRepayAmount();

    asset.safeTransferFrom(msg.sender, address(this), amount);
    emit_DebtRepaid(msg.sender, amount);

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

    hooks.onRepay(amount, state, _runtimeConstant(0x24));

    _writeState(state);
}

Also in repayOutstandingDebt():

  function repayOutstandingDebt() external nonReentrant sphereXGuardExternal {
    MarketState memory state = _getUpdatedState();
    uint256 outstandingDebt = state.totalDebts().satSub(totalAssets());
    _repay(state, outstandingDebt, 0x04);
    _writeState(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 these functions 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 guidelines: "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)."

Also worth noting is that this was completely missed in the v1 audit competition last year (though I did not participate in the contest).

Proof of Concept

Unlike the deposit() and borrow() functions, which include the necessary check, a sanctioned address can bypass the sanction check by directly calling the repay() or repayOutstandingDebt() functions.

‹‹Test output logs›› :

Ran 2 tests for test/bug.t.sol:WildcatMarketSanctionTest

[PASS] test_repay_SanctionedAddressCanRepay() (gas: 82718)
Logs:
  computeCreateAddress is deprecated. Please use vm.computeCreateAddress instead.
  Previous market balance: 200.000000000000000000
  New market balance: 400.000000000000000000
  Repaid amount: 200.000000000000000000

[PASS] test_repayOutstandingDebt_SanctionedAddressCanRepay() (gas: 94341)
Logs:
  computeCreateAddress is deprecated. Please use vm.computeCreateAddress instead.
  Previous market balance: 200.000000000000000000
  New market balance: 1000.000000000000000000
  Outstanding debt: 800.000000000000000000

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 {
    address sanctionedAddress = address(0x3);

    function setUp() public override {
        super.setUp();
        // Deposit 1000 tokens from alice
        _deposit(alice, 1000e18);

        // Borrower borrows 800 tokens (80% of the deposit)
        vm.prank(borrower);
        market.borrow(800e18);

        // Sanction the address
        sanctionsSentinel.sanction(sanctionedAddress);
    }

    function test_repay_SanctionedAddressCanRepay() public {
        // Amount to repay: 200 tokens (25% of borrowed amount)
        uint256 repayAmount = 200e18;

        // Mint tokens to the sanctioned address for repayment
        asset.mint(sanctionedAddress, repayAmount);

        vm.startPrank(sanctionedAddress);

        // Approve the market to spend tokens on behalf of the sanctioned address
        asset.approve(address(market), repayAmount);

        // Get the market balance before repayment
        uint256 prevMarketBalance = asset.balanceOf(address(market));

        // Attempt to repay from the sanctioned address
        // This should revert according to the invariant, but it doesn't
        market.repay(repayAmount);

        // Get the new market balance after repayment
        uint256 newMarketBalance = asset.balanceOf(address(market));

        // Log the balances and repaid amount
        emit log_named_decimal_uint("Previous market balance", prevMarketBalance, 18);
        emit log_named_decimal_uint("New market balance", newMarketBalance, 18);
        emit log_named_decimal_uint("Repaid amount", repayAmount, 18);

        // Assert that the market balance has increased, which shouldn't happen for a sanctioned address
        assertTrue(newMarketBalance > prevMarketBalance, "Sanctioned address should not be able to repay");

        // Additional assertion to check the exact increase matches the repaid amount
        assertEq(newMarketBalance - prevMarketBalance, repayAmount, "Market balance increase should match repaid amount");

        vm.stopPrank();
    }

    function test_repayOutstandingDebt_SanctionedAddressCanRepay() public {
        uint256 outstandingDebt = market.totalDebts() - market.totalAssets();
        asset.mint(sanctionedAddress, outstandingDebt);

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

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

        // this should revert, but it doesn't
        market.repayOutstandingDebt();

        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("outstanding debt", outstandingDebt, 18);

        assertTrue(newMarketBalance > prevMarketBalance, "sanctioned address should not be able to repay outstanding debt");
    }
}

Tools used

Foundry

Recommendation:

Implement a sanction check in the repay() and repayOutstandingDebt() functions, similar to the check in the borrow function.

if (_isFlaggedByChainalysis(msg.sender)) {

  revert_rapayWhileSanctioned();
}

Assessed type

Access Control

laurenceday commented 2 months ago

Please see https://github.com/code-423n4/2024-08-wildcat-findings/issues/20#issuecomment-2359438506 for initial context on repay (repayOutstandingDebt is simply a further refinement of the same mechanism). This is precisely the issue that made us aware that we were probably going to get grief over it, which is mildly frustrating.

We do not consider this finding valid, nor is it a change that we are interested in making, as it would be a waste of gas for something can be sidestepped simply by performing an ERC-20 transfer and a state update. repay is somewhat unique compared to other functions that do have the sanction-check in that it doesn't impact the market token supply and -- as mentioned in the comment -- is simply a macro for ease of UX.

If there is an original sin here on our part, it is that the invariant didn't state that the market state changes we're actually concerned with involve market token supply, rather than the scale factor and liquid reserve updates. In any event, even if pedantry dictates that the finding must stand because of the wording, it is not a high (there are literally no funds at risk here, and in practice no one that is sanctioned is going to be interacting with a credit contract anyway since they'll very likely be KYC'd).

More generally, we flagged this warden early on as leaning on LLMs to bug-hunt, to the point where we decided to cease engaging with them after they asked a bunch of questions that indicated minimal familiarity with Solidity and noticed that their Discord account was created two days before the Wildcat audit started.

We'd happily accept this as a QA (and I accepted as much in our thread with them): however, calling it a high or medium because of wordplay games [he readily said he was just playing the game] would be taking the mick out of the efforts of other wardens.

3docSec commented 1 month ago

Contract does not follow the spec. I get that a "key invariant" is quoted, but I see no relation to the C4 severity categorization, I agree with QA.

c4-judge commented 1 month ago

3docSec changed the severity to QA (Quality Assurance)