Summary
As per docs liquidation timelines are:
Assuming flagShort() was successful,
- firstLiquidationTime is 10hrs, secondLiquidationTime is 12hrs, resetLiquidationTime is 16hrs (these are modifiable)
- Between updatedAt and firstLiquidationTime, liquidate() isn't callable.
- Between firstLiquidationTime and secondLiquidationTime, liquidate() is only callable by short.flagger.
- Between secondLiquidationTime and resetLiquidationTime, liquidate() is callable by anyone.
- Past resetLiquidationTime, liquidate() isn't callable.
The first timeline of firstLiquidationTime is 10hrs, secondLiquidationTime is 12hrs is not properly implemented due to logic error in _canLiquidate(). This function is internally called whenever an external user calls liquidate().
Vulnerability Details & Impact
L384 calculates timeDiff as:
File: contracts/facets/MarginCallPrimaryFacet.sol
384 @> uint256 timeDiff = LibOrders.getOffsetTimeHours() - m.short.updatedAt;
LibOrders.getOffsetTimeHours() returns:
File: contracts/libraries/LibOrders.sol
30 function getOffsetTimeHours() internal view returns (uint24 timeInHours) {
31 @> return uint24(getOffsetTime() / 1 hours);
32 }
Any timestamp is rounded-down to the hour. So 10 hours 1 minute would be considered as 10 hours.
Important to note that even the updatedAt value of an order is being set using LibOrders.getOffsetTimeHours() instead of using an exact timestamp, as can be seen here.
Let's see the problem this poses with an example:
Assume that the "starting time" or contract deployment time is at midnight i.e 00:00 .
Suppose short gets updated at timestamp: 5:01 . Protocol will store this in rounded-down form as `updatedAt` = 5:00
Suppose the protocol allows a user to exit (or flag, or some action) this short after `X` hours. Let X = 2.
So the user should ideally be able to exit after 7:01.
However, when he tries, at 7:02, even this timestamp is recorded using `LibOrders.getOffsetTimeHours()` (rounded-down) and stored as 7:00. The `timeDiff` = 7:00 - 5:00 = 2 hours.
This is less than equal to X (or `firstLiquidationTime` or some deadline) i.e `timeDiff <= X`, so he won't be allowed to perform the action.
The user will have to wait another 58 minutes till 8:00 so that timeDiff is 3 hours.
The relevant piece of code inside _canLiquidate() is here:
File: contracts/facets/MarginCallPrimaryFacet.sol
384 @> uint256 timeDiff = LibOrders.getOffsetTimeHours() - m.short.updatedAt;
385 uint256 resetLiquidationTime = LibAsset.resetLiquidationTime(m.asset);
386
387 if (timeDiff >= resetLiquidationTime) {
388 return false;
389 } else {
390 uint256 secondLiquidationTime = LibAsset.secondLiquidationTime(m.asset);
391 @> bool isBetweenFirstAndSecondLiquidationTime = timeDiff
392 @> > LibAsset.firstLiquidationTime(m.asset) && timeDiff <= secondLiquidationTime
393 && s.flagMapping[m.short.flaggerId] == msg.sender;
394 @> bool isBetweenSecondAndResetLiquidationTime =
395 @> timeDiff > secondLiquidationTime && timeDiff <= resetLiquidationTime;
396 if (
397 !(
398 (isBetweenFirstAndSecondLiquidationTime)
399 || (isBetweenSecondAndResetLiquidationTime)
400 )
401 ) {
402 revert Errors.MarginCallIneligibleWindow();
403 }
404
405 return true;
406 }
Hence, the impact is the following (PoCs follow in the next section):
Impact #1: The flagger who had flagged the short and waited for 10 hours to liquidate it can not do so in the first hour i.e. between 10-11th hour. This also means the shorter has 11 hours instead of 10 hours after being flagged, to correct his cRatio.
Impact #2: Anyone (flagger or non-flagger) should be allowed to liquidate between 12-16th hours as per docs. However, a non-flagger is not able to do so between 12-13th hour. Effectively, non-flaggers get only 3 hours instead of the expected 4 hours to liquidate a flagged short because at the stroke of the 16th hour, short's flag is reset (This condition for now, works correctly in _canLiquidate(). This works correctly because resetLiquidationTime is currently taken to be a full hour with no minutes i.e 16 hours. Had it been 16 hours 30 minutes, we could have seen a bug here too).
PoC
Create a file inside test/ folder by the name of IncorrectCanLiquidate.t.sol and paste the following code. It has 2 tests along with a few helper functions:
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";
contract IncorrectCanLiquidate is OBFixture {
using U256 for uint256;
using U88 for uint88;
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);
}
function test_01_FlaggerCanNotLiquidateBetween10And11Hours() public {
address _flagger = makeAddr("_flagger_");
_flagShortAndSkipTime({timeToSkip: 10 hours + 1 seconds, flagger: _flagger});
vm.expectRevert(Errors.MarginCallIneligibleWindow.selector);
vm.prank(_flagger);
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
rewind(10 hours + 1 seconds);
skip(10 hours + 59 minutes);
vm.expectRevert(Errors.MarginCallIneligibleWindow.selector);
vm.prank(_flagger);
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
rewind(10 hours + 59 minutes);
skip(10 hours + 1 hours);
vm.prank(_flagger);
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
}
function test_02_NonFlaggerCanNotLiquidateBetween12And13Hours() public {
address _flagger = makeAddr("_flagger_");
address _nonFlagger = makeAddr("_non_flagger_");
_flagShortAndSkipTime({timeToSkip: 12 hours + 1 seconds, flagger: _flagger});
vm.expectRevert(Errors.MarginCallIneligibleWindow.selector);
vm.prank(_nonFlagger);
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
rewind(12 hours + 1 seconds);
skip(12 hours + 59 minutes);
vm.expectRevert(Errors.MarginCallIneligibleWindow.selector);
vm.prank(_nonFlagger);
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
rewind(12 hours + 59 minutes);
skip(12 hours + 1 hours);
vm.prank(_nonFlagger);
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
}
}
Run forge test --mt test_01_FlaggerCanNotLiquidateBetween10And11Hours -vv to verify Impact#1.
Run forge test --mt test_02_NonFlaggerCanNotLiquidateBetween12And13Hours -vv to verify Impact#2.
Both tests pass, but should have failed as per protocol specifications.
Tools Used
Manual review, foundry.
Recommendations
Instead of doing these calculations in hours, they should be done in seconds to avoid rounding error.
Informational note to the development team
I can see pre-existing tests which could have spotted this issue as the comment provided next to these lines is //10hrs 1 second:
File: test/MarginCallPrimary.t.sol
1187 @> skipTimeAndSetEth(TEN_HRS_PLUS, 730 ether);
However, the constant TEN_HRS_PLUS is defined as:
File: test/utils/ConstantsTest.sol
15 @> uint256 public constant TEN_HRS_PLUS = 10 hours + 1 hours;
16 uint256 public constant TWELVE_HRS_PLUS = 12 hours + 1 hours;
17 uint256 public constant SIXTEEN_HRS_PLUS = 16 hours + 1 hours;
Maybe you meant it to be uint256 public constant TEN_HRS_PLUS = 10 hours + 1 seconds;? Same for the other constants too?
Or maybe you already encountered this issue as is evident by the comment here?
Pointing this out as you may be able to spot more bugs after correcting this.
Additional note on duplicativeness
A note to the judges, in case it helps move things faster: I have raised two other bugs with the following titles-
Bug-A is inside a different function, flagShort(), and the fix would need to be applied there. The impact is different too, which relates to the inability of users to re-flag a short.
Bug-B is also different in the sense that it is inside 2 different functions disburseCollateral() & _distributeYield(), and the impact is related to loss of yield.
Their similarity with the current bug is that they happen due to incorrect placement of LibOrders.getOffsetTimeHours(). Raised them separate from this one because they seem like totally different/unique business cases too. Leaving it up to you for further analysis.
