DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

Flag does not reset on favorable price movement

t0x1c

Summary

As per docs :

"

Primary Margin Call Method

In the Primary Margin Call Method, when an account is below 200% and is flagged for liquidation, a liquidation timer of 10 hours begins on the account to give a window of opportunity to the flagged shorter to fix their CR and stop the liquidation countdown.

To halt the liquidation timer and remove the flag, the shorter must reach the target maintenance margin CR (200%) again by either the price moving favorably or by adding additional collateral.

"

Basically the flag is expected to be reset by the price moving favorably.

This behaviour is not followed by the protocol.

Vulnerability Details

Imagine the following flow of events:

  • #1. A short is flagged by a flagger as its cRatio < 4

  • #2. Shorter now has 10 hours. If cRatio is still < 4 after 10 hours, flagger has a 2 hour window to liquidate the short.

  • #3. Assume that the price moves in favour of the shorter and at the 8th hour, cRatio > 4 is achieved. The shorter himself did not take any action to do this. He just bet on the market & the price movement to save him. Or you can imagine that in the time he was arranging funds to fund the short, the market moved favourably and hence he did not need to do any action.

  • #4. 10 hours pass. Obviously, flagger won't be able to liquidate now as cRatio > 4.

  • #5. The flag should also have been reset at the 8th hour. This is necessary so that in case cRatio again dips below 4 in the future, one has to flag the short again and wait for 10 hours before attempting liquidation.

    However, the flag is not reset by the protocol in the above scenario.

Impact-1

Since the flag never got reset, if the price moves unfavourably again such that cRatio < 4, then -

  • between 10-12 hour window, flagger can liquidate the short immediately, with no waiting period.

  • between 12-16 hour window, anyone can liquidate the short immediately, with no waiting period.

Impact-2

Let's now imagine that:

  • #1. 16 hours pass. As sepcified in the docs flag should be reset, forcing margin-callers to re-flag the short:

In the case where the 16 hour window has passed, margin callers must re-flag the short to begin the process again to liquidate the partial debt position.

Since the flag never got reset, if the price ever moves unfavourably after the 16 hour window such that cRatio < 4, then -

  • anyone can liquidate the short immediately, with no waiting period.

PoC

Create a file inside test/ folder by the name of FlagNotResetOnPriceMovement.t.sol and paste the following code. It has 3 tests along with a few helper functions:

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.21;
​
import {Constants} from "contracts/libraries/Constants.sol";
import {Errors} from "contracts/libraries/Errors.sol";
import {U256, U88} from "contracts/libraries/PRBMathHelper.sol";
import {OBFixture} from "test/utils/OBFixture.sol";
import {STypes, SR} from "contracts/libraries/DataTypes.sol";
import {LibShortRecord} from "contracts/libraries/LibShortRecord.sol";
​
import {console} from "contracts/libraries/console.sol";
​
contract FlagNotResetOnPriceMovement is OBFixture {
using U256 for uint256;
using U88 for uint88;
using LibShortRecord for STypes.ShortRecord;
​
function _initialAssertStruct() public {
r.ethEscrowed = 0;
r.ercEscrowed = DEFAULT_AMOUNT;
assertStruct(receiver, r);
s.ethEscrowed = 0;
s.ercEscrowed = 0;
assertStruct(sender, s);
e.ethEscrowed = 0;
e.ercEscrowed = 0;
assertStruct(extra, e);
t.ethEscrowed = 0;
t.ercEscrowed = 0;
assertStruct(tapp, t);
}
​
function _prepareAsk(uint80 askPrice, uint88 askAmount) public {
fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender);
​
fundLimitAskOpt(askPrice, askAmount, receiver);
assertEq(getShortRecordCount(sender), 1);
assertEq(
getShortRecord(sender, Constants.SHORT_STARTING_ID).collateral,
DEFAULT_AMOUNT.mulU88(DEFAULT_PRICE) * 6
);
_initialAssertStruct();
}
​
function _checkFlaggerAndUpdatedAt(
address _shorter,
uint16 _flaggerId,
uint256 _updatedAt
) public {
STypes.ShortRecord memory shortRecord =
getShortRecord(_shorter, Constants.SHORT_STARTING_ID);
​
assertEq(shortRecord.flaggerId, _flaggerId, "flagger id mismatch");
assertEq(shortRecord.updatedAt, _updatedAt, "updatedAt mismatch");
}
​
function _flagShortAndSkipTime(uint256 timeToSkip, address flagger) public {
_prepareAsk({askPrice: DEFAULT_PRICE, askAmount: DEFAULT_AMOUNT});
_setETH(2666 ether);
_checkFlaggerAndUpdatedAt({_shorter: sender, _flaggerId: 0, _updatedAt: 0});
vm.prank(flagger);
diamond.flagShort(asset, sender, Constants.SHORT_STARTING_ID, Constants.HEAD);
uint256 flagged = diamond.getOffsetTimeHours();
skipTimeAndSetEth({skipTime: timeToSkip, ethPrice: 2666 ether});
_checkFlaggerAndUpdatedAt({_shorter: sender, _flaggerId: 1, _updatedAt: flagged});
assertSR(
getShortRecord(sender, Constants.SHORT_STARTING_ID).status, SR.FullyFilled
);
depositEth(tapp, DEFAULT_TAPP);
}
​
/* solhint-disable no-console */
function test_01_flagDoesNotResetOnFavorablePriceMovement() public {
address _flagger_01 = makeAddr("_flagger_01");
_flagShortAndSkipTime({timeToSkip: 8 hours, flagger: _flagger_01}); // cR < 4 here, hence flagged
STypes.ShortRecord memory theShortRecord =
getShortRecord(sender, Constants.SHORT_STARTING_ID);
​
// favorable price movement after 8 hours
_setETH(2700 ether); // cR > 4
skip(3 hours); // 11 hours have passed since flagging. Eligible for liquidation attempt.
_setETH(2700 ether); // avoids evm revert. cR still > 4
​
// should be true
bool isFlagged = theShortRecord.flaggerId != 0;
console.log("isFlagged (should be true) =", isFlagged);
​
vm.prank(_flagger_01);
vm.expectRevert(Errors.SufficientCollateral.selector);
// Correctly reverts as cR > 4
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
​
// should be false
isFlagged = theShortRecord.flaggerId != 0;
console.log("isFlagged (should be false) =", isFlagged); // @audit-issue : true; still not unflagged!
assertEq(isFlagged, false, "Short's flag was not reset!");
}
​
/* solhint-disable no-console */
function test_02_ImmediateLiquidationPossibleOnUnfavorablePriceMovement() public {
address _flagger_01 = makeAddr("_flagger_01");
address _anyone = makeAddr("_anyone_");
_flagShortAndSkipTime({timeToSkip: 8 hours, flagger: _flagger_01}); // cR < 4 here, hence flagged
STypes.ShortRecord memory theShortRecord =
getShortRecord(sender, Constants.SHORT_STARTING_ID);
​
// favorable price movement after 8 hours
_setETH(2700 ether); // cR > 4
skip(3 hours); // 11 hours have passed since flagging. Eligible for liquidation attempt.
_setETH(2700 ether); // avoids evm revert. cR still > 4
​
// should be true
bool isFlagged = theShortRecord.flaggerId != 0;
console.log("isFlagged (should be true) =", isFlagged);
​
vm.prank(_flagger_01);
vm.expectRevert(Errors.SufficientCollateral.selector);
// Correctly reverts as cR > 4
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
​
// should be false
isFlagged = theShortRecord.flaggerId != 0;
console.log("isFlagged (should be false) =", isFlagged); // @audit-issue : true; still not unflagged!
​
skip(3 hours); // 14 hours passed since flagging. Anyone can attempt liquidation.
_setETH(2700 ether); // avoids evm revert. cR still > 4
​
vm.prank(_anyone);
vm.expectRevert(Errors.SufficientCollateral.selector);
// Correctly reverts as cR > 4
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
​
skip(2 minutes);
_setETH(2666 ether); // cR < 4 due to unfavorable price movement.
​
vm.prank(_anyone);
// @audit-issue : ANYONE is able to liquidate (incorrectly), without re-flagging and waiting for 10 hours! Should not be possible.
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
}
​
/* solhint-disable no-console */
function test_03_flagNotResetAfter16Hours() public {
address _flagger_01 = makeAddr("_flagger_01");
address _anyone = makeAddr("_anyone_");
_flagShortAndSkipTime({timeToSkip: 8 hours, flagger: _flagger_01}); // cR < 4 here, hence flagged
STypes.ShortRecord memory theShortRecord =
getShortRecord(sender, Constants.SHORT_STARTING_ID);
​
// favorable price movement after 8 hours
_setETH(2700 ether); // cR > 4
skip(3 hours); // 11 hours have passed since flagging. Eligible for liquidation attempt.
_setETH(2700 ether); // avoids evm revert. cR still > 4
​
// should be true
bool isFlagged = theShortRecord.flaggerId != 0;
console.log("isFlagged (should be true) =", isFlagged);
​
vm.prank(_flagger_01);
vm.expectRevert(Errors.SufficientCollateral.selector);
// Correctly reverts as cR > 4
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
​
skip(3 hours); // 14 hours passed since flagging. Anyone can attempt liquidation.
_setETH(2700 ether); // avoids evm revert. cR still > 4
​
vm.prank(_anyone);
vm.expectRevert(Errors.SufficientCollateral.selector);
// Correctly reverts as cR > 4
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
​
skip(4 hours); // 18 hours passed since flagging. Flag should have been reset.
_setETH(2700 ether); // avoids evm revert. cR still > 4
​
// should be false
isFlagged = theShortRecord.flaggerId != 0;
console.log("isFlagged (should be false) =", isFlagged); // @audit-issue : true; still not unflagged!
​
skip(1 minutes);
_setETH(2666 ether); // cR < 4 due to unfavorable price movement.
​
vm.prank(_anyone);
// @audit-issue : ANYONE is able to liquidate (incorrectly) even after 16 hours, without re-flagging and waiting for 10 hours! Should not be possible.
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
}
}
​

  • Test-1: Run forge test --mt test_01_flagDoesNotResetOnFavorablePriceMovement -vv to verify that the flag is never reset.

    Output:

Logs:
isFlagged (should be true) = true
isFlagged (should be false) = true
Error: Short's flag was not reset!
Error: a == b not satisfied [bool]
Left: true
Right: false
​
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 133.72ms

  • Test-2: Run forge test --mt test_02_ImmediateLiquidationPossibleOnUnfavorablePriceMovement -vv to verify the aforementioned Impact-1.

    The test passes, but should have failed as per protocol specifications.

  • Test-3: Run forge test --mt test_03_flagNotResetAfter16Hours -vv to verify the aforementioned Impact-2.

    The test passes, but should have failed as per protocol specifications.

Tools Used

Manual review, foundry.

Recommendations

  • Whenever a short is flagged, its price movement needs to be tracked so that its flag can be correctly reset whenever it crosses a healthy cRatio back again. This might need altogether additional new functions to be implemented. An idea: Users can be incentivized to reset a short's flag when the right conditions are met (just like they are incentivized to flag a short).

  • Another approach could be that at every liquidation or flagging attempt of a short, the code not only checks whether the short is already flagged, but also the timestamp of the flagging. If it is beyond 16 hours, flag resets. This approach however, will only address Impact-2.

Updates

Lead Judging Commences

0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-176

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.