howlbot-integration[bot] commented 2 months ago

Lines of code

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

Vulnerability details

Impact

When a borrower calls the repayOutstandingDebt() function, the expected behavior is that all outstanding debts should be repaid in full. However, due to a flaw in the implementation, specifically not processing expired or batched withdrawals after the debt repayment, the debt accumulates again after the repayment. This results in the system incurring more debt than expected, creating a financial discrepancy and making the repayment function ineffective as borrower will still have to pay more in a case where everyone has queued withdrawals.

In essence, this issue allows debt to persist even after a full repayment is attempted, leading to inconsistent system behavior where the borrower continues to owe money even after settling their outstanding obligations.

Proof of Concept

The following two test setups demonstrate the bug in the repayOutstandingDebt() function:

Test Case 1: Debt Repayment Without Updating

function test_repayOutstandingDebt() external {
    _depositBorrowWithdraw(alice, 1e18, 8e17, 1e18);  // Deposit and withdraw setup
    asset.mint(address(this), 8e17);  // Mint tokens for debt repayment
    asset.approve(address(market), 8e17);  // Approve repayment
    vm.expectEmit(address(market));
    emit DebtRepaid(address(this), 8e17);  // Expect debt repayment event

    MarketState memory state = pendingState();  // Get current market state

    market.repayOutstandingDebt();  // Repay the outstanding debt

    fastForward(1);  // Move time forward by 1 second
    assertEq(market.totalDebts() - market.totalAssets(), 0);  // Check if debt is cleared
}

This test case fails because, 1 second after repayment, debt accumulates again. The reason is that pending withdrawals or accrued fees are not processed before the repayment, causing debt to resurface immediately.

Failing tests:
Encountered 1 failing test in test/market/WildcatMarket.t.sol:WildcatMarketTest
[FAIL. Reason: assertion failed: 241095890410958 != 0] test_repayOutstandingDebt() (gas: 1237051)

Test Case 2: Debt Repayment with Market Update

function test_repayOutstandingDebt() external {
    _depositBorrowWithdraw(alice, 1e18, 8e17, 1e18);  // Deposit and withdraw setup
    asset.mint(address(this), 8e17);  // Mint tokens for debt repayment
    asset.approve(address(market), 8e17);  // Approve repayment
    vm.expectEmit(address(market));
    emit DebtRepaid(address(this), 8e17);  // Expect debt repayment event

    MarketState memory state = pendingState();  // Get current market state

    market.repayOutstandingDebt();  // Repay the outstanding debt
    market.updateState();  // Update the market state after repayment

    fastForward(1 days);  // Move time forward by 1 day
    assertEq(market.totalDebts() - market.totalAssets(), 0);  // Check if debt is cleared
}

Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 39.72ms (17.64ms CPU time)

Ran 1 test suite in 76.31ms (39.72ms CPU time): 2 tests passed, 0 failed, 0 skipped (2 total tests)

In this case, the debt repayment works as expected because the updateState() function is called after repayment, ensuring that all expired and batched withdrawals are processed before the debt is recalculated. This test passes as the total debt does not accumulate again.

Referencing another implementation in the code

 function repayAndProcessUnpaidWithdrawalBatches(
    uint256 repayAmount,
    uint256 maxBatches
  ) public nonReentrant sphereXGuardExternal {

@audit>>    // Repay before updating state to ensure the paid amount is counted towards
@audit>>     // any pending or unpaid withdrawals.
    if (repayAmount > 0) {
      asset.safeTransferFrom(msg.sender, address(this), repayAmount);
      emit_DebtRepaid(msg.sender, repayAmount);
    }

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

    // Use an obfuscated constant for the base calldata size to prevent solc
    // function specialization.
    if (repayAmount > 0) hooks.onRepay(repayAmount, state, _runtimeConstant(0x44));

Tools Used

Manual code review
Unit tests in Solidity using the foundry test framework

Recommended Mitigation Steps

To resolve this issue, the system should process all pending withdrawals and update the market state after repaying the debt. This can be achieved by modifying the repayOutstandingDebt() function to include the state update immediately after repayment.

Suggested Fix:

function repayOutstandingDebt() external nonReentrant sphereXGuardExternal {
    MarketState memory state = _getUpdatedState();  // Ensure state is updated before repayment

    uint256 outstandingDebt = state.totalDebts().satSub(totalAssets());
    _repay(state, outstandingDebt, 0x04);  // Repay the outstanding debt

++   // Process expired and batched withdrawals ADD LOGIC

    _writeState(state);  // Write updated state to storage
}

Assessed type

Error

3docSec commented 1 month ago

Rewarding this one at 50% because, unlike other dupes, it does not report the same issue in repayDelinquentDebt

c4-judge commented 1 month ago

3docSec marked the issue as partial-50

Tomiwasa0 commented 1 month ago

Thank you for judging @3docSec ,

i submitted repayDelinquentDebt as a separate issue of this kind. kindly check issue 59. Well done