code-423n4 / 2024-08-wildcat-findings

3 stars 1 forks source link

Markets do not update delinquency timer correctly if the account goes underwater through interest accrual #89

Closed howlbot-integration[bot] closed 2 months ago

howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarketBase.sol#L406-L465 https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarketBase.sol#L541-L542

Vulnerability details

Impact

Whenever there is an interaction with a protocol, the protocol updates it's own state since the last interest accrual happened. One of the important parts of the state is calculating if the account of the borrower is underwater. In such cases, the protocol starts counting the delinquency timer, and if the timer passes a threshold the borrower has to pay a penalty.

In case an account has gone underwater through interest accrual from borrower to lenders, the protocol does not count up the delinquency timer until the next state accrual happens. This means that if the distance between two interactions are far away from each other, protocol might miss-calculate the time the borrower has been underwater.

This is important especially because: (1) There might be many borrowing pools since the liquidity is not centralized in wildcat, therefore, there is no need for constant interaction with the protocol. If the providers have provided their assets, and borrower has borrowed what they need, there will be instances that no one might send a transaction to the protocol for days, weeks, or even months! (Can be seen in V1 Markets)

(2) The users depend on Wildcat to calculate the correct amount in such cases, however, updateScaleFactorAndFees considers the duration since last time stamp to be either all underwater, or all healthy. The Wildcat protocol is misusing the library and therefore causing incorrect calculations.

(3) Every second in delinquency counts twice if the market isn't closed, once when it is counting up and once when it is counting down.

Proof of Concept

The PoC below shows that even if a large amount of time passes, the wildcat would not start counting the time until the next interaction with the protocol:

  function test_borrowDelinquency() external {
    vm.prank(alice);
    market.depositUpTo(50_000e18);
    assertEq(market.borrowableAssets(), 40_000e18, 'borrowable should be 40k');
    vm.prank(borrower);
    market.borrow(40_000e18); // borrow the max amount, the reserve ratio is 20% here

    vm.warp(block.timestamp + 1000000000); // During this time the market is definitely delinquent

    market.updateState();

    vm.warp(block.timestamp + 1000);

    MarketState memory state = market.currentState();

    assertEq(state.timeDelinquent, 1000);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

In case the last time protocol was contacted with, the borrower wasn't underwater, and now is underwater, the protocol needs to calculate when that happened through interest accrual, and break the update into two sections.

Assessed type

Timing

c4-judge commented 1 month ago

3docSec marked the issue as unsatisfactory: Invalid