Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repayDeliquentDebt is not effective, as the market will become delinquent again on next block #73

Closed
howlbot-integration bot opened this issue Sep 20, 2024 · 6 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-62 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) 🤖_71_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/market/WildcatMarket.sol#L172-L172
https://github.com/code-423n4/2024-08-wildcat/blob/main/src/market/WildcatMarketBase.sol#L541-L542

Vulnerability details

Summary

repayDelinquentDebt allow a borrower to repay the exact amount necessary to cover the delinquency, i.e make the totalAssets in the market equal to the liquidityRequired.
But as fees accrues every seconds (and new block), the market will be delinquent again on next block.

Vulnerability details

The repayDelinquentDebt works that way:

  1. The market state is updated in _getUpdatedState(). During this call, as the market is delinquent, delinquency is applied (either by applying fees or increasing the delinquent timer)
  2. Then the exact amount to make the market not delinquent is computed as delinquentDebt
  3. The amount is repaid (a transfer of the delinquentDebt is made from the borrower to the market)
  4. Finally, the state.isDelinquent variable is set to false in _writeState as state.liquidityRequired() == totalAssets() now.
File: src/market/WildcatMarket.sol
186:   function repayDelinquentDebt() external nonReentrant sphereXGuardExternal {
187:     MarketState memory state = _getUpdatedState();
188:     uint256 delinquentDebt = state.liquidityRequired().satSub(totalAssets());
189:     _repay(state, delinquentDebt, 0x04);
190:     _writeState(state);
191:   }
File: src/market/WildcatMarketBase.sol
540:   function _writeState(MarketState memory state) internal {
541:     bool isDelinquent = state.liquidityRequired() > totalAssets();
542:     state.isDelinquent = isDelinquent; 
543:	// .... 
544: // ....	

The issue here, is that on the next block that will occur, any stateful interaction (starts with _getUpdatedState() and ends with _writeState) with the market will increase the fees and reserve ratio in _getUpdatedState(), increasing state.liquidityRequired(), making the market delinquent again when _writeState is called at the end.

Impact

This will cause the market to still accrue delinquency fees, causing a loss for the borrower.
The repayDelinquentDebt purpose is defeated by the fact this only remove the delinquency of the market for one block, which is not effective to help a borrower solve the issue effectively, who will see the delinquency continue to affect its market, causing additional cost that could have been avoided by adding a buffer of repayment.

Proof of Concept

Add this test to test/market/WildcatMarket.t.sol:

  function testAudit_repayDelinquentDebtNotEffective() external {
    _depositBorrowWithdraw(alice, 1e18, 8e17, 2e17); // 20% of 8e17
    asset.mint(address(this), 1.6e17);
    asset.approve(address(market), 1.6e17);
		
	// market is deliquent
	MarketState memory state = market.currentState();
	assertEq(true, state.isDelinquent);

		// borrower repay the delinquecy
    market.repayDelinquentDebt();
		
		// market is no more deliquent as it has been repaid
	market.updateState();
	state = market.currentState();
	assertEq(false, state.isDelinquent);

	console.log("--- state after repayDelinquentDebt ---");
	console.log("---------------------------------------");
	console.log("isDelinquent: ", state.isDelinquent);
	console.log("totalAssets: %e", market.totalAssets());
	console.log("liquidityRequired: %e\n", state.liquidityRequired());

	// increasing timestamp by 12s to simulate one new block 
	vm.warp(block.timestamp + 12);

	// as exected, market is deliquent as even 1 wei of added interest will make the market delinquent again
	market.updateState();
	state = market.currentState();
	assertEq(true, state.isDelinquent);

	console.log("---  state after one block elapsed  ---");
	console.log("---------------------------------------");
	console.log("isDelinquent: ", state.isDelinquent);
	console.log("totalAssets: %e", market.totalAssets());
	console.log("liquidityRequired: %e", state.liquidityRequired());
  }

Ouput for the test:

Ran 1 test for test/market/WildcatMarket.t.sol:WildcatMarketTest
[PASS] testAudit_repayDelinquentDebtNotEffective() (gas: 1070934)
Logs:
  computeCreateAddress is deprecated. Please use vm.computeCreateAddress instead.
  --- state after repayDelinquentDebt ---
  ---------------------------------------
  isDelinquent:  false
  totalAssets: 3.6e17
  liquidityRequired: 3.6e17

  ---  state after one block elapsed  ---
  ---------------------------------------
  isDelinquent:  true
  totalAssets: 3.6e17
  liquidityRequired: 3.60000009132420091e17

Tools Used

Manual review

Recommended Mitigation Steps

The goal is to give more time such that the timeDelinquent timer can decrease when the delinquency is repaid, and allow the borrower to act.
I see 2 solutions:

  1. Add a fixed repayment buffer to cover for the increase in fees on next blocks
  2. Add an input value allowing the to the amount of buffer he wants to add

This would also cover the 1-2 wei corner case of stETH (a rebasing token) in case a market uses it as its asset.

Assessed type

Math

@howlbot-integration howlbot-integration bot added 3 (High Risk) Assets can be stolen/lost/compromised directly 🤖_71_group AI based duplicate group recommendation bug Something isn't working duplicate-19 sufficient quality report This report is of sufficient quality labels Sep 20, 2024
howlbot-integration bot added a commit that referenced this issue Sep 20, 2024
@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Oct 3, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 3, 2024

3docSec marked the issue as unsatisfactory:
Invalid

@InfectedIsm
Copy link

InfectedIsm commented Oct 10, 2024

Hey, thanks a lot for your timely judging!
Looks like this issue has been wrongly duplicated during the validation phase, and when #19 has been set to "unsatisfactory", so did my issue:
#19 is talking about wrong delinquency fees applied, while my issue is talking about the repayDeliquentDebt function being not effective, as the market will become delinquent for the next block again.
The right duplication should be #100
And it seems that
Thanks a lot

@c4-judge
Copy link
Contributor

3docSec marked the issue as not a duplicate

@c4-judge
Copy link
Contributor

3docSec marked the issue as duplicate of #62

@c4-judge c4-judge added duplicate-62 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) and removed unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Oct 14, 2024
@c4-judge
Copy link
Contributor

3docSec marked the issue as partial-50

@c4-judge
Copy link
Contributor

3docSec changed the severity to 2 (Med Risk)

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-62 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) 🤖_71_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

2 participants