search

code-423n4 / 2023-10-wildcat-findings

12 stars 9 forks source link

Borrower escapes delinquency penalty if no intermittent action happens #706

Closed c4-submissions closed 10 months ago

c4-submissions commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarket.sol#L26

Vulnerability details

Impact

Once the market goes below required reserves, it is marked as delinquent only if an updateState() action happens. Actions like market.updateState(), executeWithdrawal(), deposit(), etc. have to happen else the protocol remains unaware of the market's delinquency and no penalty fees is applied on the borrower. The borrower can repay before any state updates happen and escape a penalty altogether.

Proof of Concept

Save the following code in a new file inside the test/market/ folder. 

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

import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol";

import "src/market/WildcatMarket.sol";
import "../shared/TestConstants.sol";
import "../helpers/Assertions.sol";
import "../shared/Test.sol";
import "../helpers/VmUtils.sol";
import "../helpers/MockController.sol";

contract t0x1cEscapePenalty is Test {
    using stdStorage for StdStorage;
    using FeeMath for MarketState;
    using SafeCastLib for uint256;
    using MathUtils for uint256;
    using FeeMath for uint256;

    MarketParameters internal t0x1cParameters;
    MockERC20 internal asset;

    function setUp() public {
        t0x1cParameters = MarketParameters({
            asset: address(0),
            namePrefix: "Wildcat ",
            symbolPrefix: "WC",
            borrower: borrower,
            controller: address(0),
            feeRecipient: feeRecipient,
            sentinel: address(sanctionsSentinel),
            maxTotalSupply: uint128(10_000e18),
            protocolFeeBips: 5000,
            annualInterestBips: 2000,
            delinquencyFeeBips: 5000,
            withdrawalBatchDuration: 0,
            reserveRatioBips: 1000,
            delinquencyGracePeriod: 0
        });

        if (address(controller) == address(0)) {
            deployController(t0x1cParameters.borrower, false, false);
        }
        t0x1cParameters.controller = address(controller);
        t0x1cParameters.asset = address(asset = new MockERC20('Token', 'TKN', 18));
        deployMarket(t0x1cParameters);
        _authorizeLender(alice);

        asset.mint(alice, 100e18);

        _approve(alice, address(market), type(uint256).max);
    }

    function _authorizeLender(address account) internal asAccount(t0x1cParameters.borrower) {
        address[] memory lenders = new address[](1);
        lenders[0] = account;
        controller.authorizeLenders(lenders);
    }

    function _approve(address from, address to, uint256 amount) internal asAccount(from) {
        asset.approve(to, amount);
    }

    function test_t0x1c_SomeAction_TimeLapse() external {
        vm.prank(alice);
        market.depositUpTo(100e18);

        vm.prank(borrower);
        market.borrow(90e18);
        assertEq(market.previousState().isDelinquent, false);

        fastForward(1 seconds);
        market.updateState();
        assertEq(market.previousState().isDelinquent, true);
    }

    function test_t0x1c_NoAction_TimeLapse() external {
        vm.prank(alice);
        market.depositUpTo(100e18);

        vm.prank(borrower);
        market.borrow(90e18);
        assertEq(market.previousState().isDelinquent, false);

        fastForward(1 seconds);
        assertEq(market.previousState().isDelinquent, false); // @audit : state remains non-delinq

        fastForward(1 hours);
        assertEq(market.previousState().isDelinquent, false); // @audit : state still remains non-delinq
        // let's mint some for borrower so that we can make him return a small amount (for testing)
        asset.mint(borrower, 0.01e18);
        // borrower sends this amount to market, before anyone updates the state
        vm.prank(borrower);
        asset.transfer(address(market), 0.01e18);
        assertEq(market.previousState().isDelinquent, false); // @audit : state is still non-delinq

        vm.prank(borrower);
        market.updateState(); 
        assertEq(market.previousState().isDelinquent, false); // @audit : state is non-delinq, borrower escapes penalty altogether
    }
}

Tools Used

Manual inspection, foundry.

Recommended Mitigation Steps

Such issues are normally overcome by protocols incentivizing participants to flag any such undercollateralization events, so that state updates can happen and protocols do not face a loss of fees. This way, a user would be inclined to hit market.updateState() and monitor any delinquent positions which can be flagged to receive an award from the protocol.

Assessed type

Other

laurenceday commented 10 months ago

Every single lending participant to a market is incentivised to hit updateState if and when they notice a scenario like this. There is no need to create an external watchdog program for it.

Moreover, this example presumes that the grace period is set to zero. This is not a rational assumption: the protocol itself is vanishingly unlikely to ever permit zero as a minimum grace period bound in any controller factory that is deployed, and even if that did happen, no sane borrower would deploy a market with that value.

c4-pre-sort commented 10 months ago

minhquanym marked the issue as primary issue

c4-pre-sort commented 10 months ago

minhquanym marked the issue as sufficient quality report

c4-sponsor commented 10 months ago

laurenceday (sponsor) disputed

c4-judge commented 10 months ago

MarioPoneder marked the issue as unsatisfactory: Overinflated severity

MarioPoneder commented 10 months ago

See also #323. Similar to liquidations in other DeFi protocols, the lenders are heavily incentivized to monitor the state and call updateState() in case. This is the intended way to avoid the the non-accrual of delinquency fees, therefore QA seems most appropriate.
(Since the original severity is High, the issue was invalidated as overinflated)