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

The right of `short.flagger` to liquidate short between `firstLiquidationTime` and `secondLiquidationTime` is incorrectly transferred to another flagger

t0x1c

Summary

As per docs :

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.


The last point can be incorrectly revoked by the protocol under the normal flow of events, violating flagger's priority liquidation time-window.

Vulnerability Details

Imagine the following flow of events:

  • #1. shortRecord_1 is flagged by flagger1 at timestamp ts. This short now has 10 hours. If cRatio is still less than primaryLiquidationCR after 10 hours, flagger1 has a 2 hour window to liquidate the short. The code sets shortRecord_1.flaggerId as 1. Read about flaggerId here:

Note: Instead of saving the flagger address, a ShortRecord saves the flaggerId instead. This is done to save space in the struct. A mapping from id to address (flagMapping) is used to lookup the address instead. There is also a corresponding AssetUser.g_flaggerId to lookup a user's flagId.

  • #2. 10 hours pass. shortRecord_1 is now eligible for liquidation by flagger1. Right about now, a different short record, shortRecord_2 is flagged by flagger2.

  • #3. Due to a logic bug in the function setFlagger(), the code sets shortRecord_2.flaggerId as 1 too, since it attempts to re-use the "inactive" flaggerId.

  • #4. The above point means that as per code, both shortRecord_1 and shortRecord_2 are now considered to be flagged by flagger2.

  • #5. Since shortRecord_1 is still in its primary liquidation window, flagger1 attempts to margin call it only to find that it reverts for him!

  • #6. Due to the above logic fallacy, flagger2 actually now has the power to liquidate shortRecord_1. He margin calls shortRecord_1 immediately and receives the rewards.

Root Cause

The current check to see if a flaggerId is "inactive" or not is incorrect. The protocol currently waits for a duration of firstLiquidationTime to pass for determining this. It should rather wait for secondLiquidationTime.

PoC

Create a file inside test/ folder by the name of TwoFlaggers.t.sol and paste the following code. Run it via forge test --mt test_TwoFlaggers -vv:

// 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, U80} from "contracts/libraries/PRBMathHelper.sol";
import {OBFixture} from "test/utils/OBFixture.sol";
import {STypes} from "contracts/libraries/DataTypes.sol";
import {LibShortRecord} from "contracts/libraries/LibShortRecord.sol";
​
import {console} from "contracts/libraries/console.sol";
​
contract TwoFlaggers is OBFixture {
using U256 for uint256;
using U88 for uint88;
using U80 for uint80;
using LibShortRecord for STypes.ShortRecord;
​
function _checkFlaggerAndUpdatedAt(
uint8 short_id,
address _shorter,
uint16 _flaggerId,
uint256 _updatedAt
) public {
STypes.ShortRecord memory shortRecord = getShortRecord(_shorter, short_id);
​
assertEq(shortRecord.flaggerId, _flaggerId, "flagger id mismatch");
assertEq(shortRecord.updatedAt, _updatedAt, "updatedAt mismatch");
}
​
/* solhint-disable no-console */
function test_TwoFlaggers() public {
address _flagger_01 = makeAddr("_flagger_01");
address _flagger_02_lucky_fella = makeAddr("_flagger_02");
​
// create orders
fundLimitShortOpt(0.00025 ether, DEFAULT_AMOUNT, sender); // default_amount = 4000 ether
fundLimitShortOpt(0.0003751 ether, DEFAULT_AMOUNT, sender);
fundLimitBidOpt(0.00025 ether, DEFAULT_AMOUNT, receiver);
fundLimitBidOpt(0.0003751 ether, DEFAULT_AMOUNT, receiver);
STypes.ShortRecord memory shortRecord_1 =
getShortRecord(sender, Constants.SHORT_STARTING_ID);
STypes.ShortRecord memory shortRecord_2 =
getShortRecord(sender, Constants.SHORT_STARTING_ID + 1);
​
skipTimeAndSetEth({skipTime: 1 minutes, ethPrice: 2666 ether}); // causing `cR < primaryLiquidationCR` for short1
// flagger1 flags short1
vm.prank(_flagger_01);
diamond.flagShort(asset, sender, Constants.SHORT_STARTING_ID, Constants.HEAD);
// verify that flaggerId == 1
_checkFlaggerAndUpdatedAt({
short_id: Constants.SHORT_STARTING_ID,
_shorter: sender,
_flaggerId: 1,
_updatedAt: diamond.getOffsetTimeHours()
});
​
// skip initial duration of 10 hours where short1 has the chance to increase its collateral
skipTimeAndSetEth({skipTime: TEN_HRS_PLUS, ethPrice: 1750 ether}); // causing `cR < primaryLiquidationCR` for short2. Short1 still remains under-collaterized.
​
// flagger2 flags short2
vm.prank(_flagger_02_lucky_fella);
diamond.flagShort(asset, sender, Constants.SHORT_STARTING_ID + 1, Constants.HEAD);
// @audit-issue : same flaggerId (1) re-used by the protocol due to which both shorts now have the same flaggerId
_checkFlaggerAndUpdatedAt({
short_id: Constants.SHORT_STARTING_ID + 1,
_shorter: sender,
_flaggerId: 1,
_updatedAt: diamond.getOffsetTimeHours()
});
assertEq(
shortRecord_1.flaggerId,
shortRecord_2.flaggerId,
"shorts have different flagger ids"
);
​
// @audit-issue :
// !!
// Values for both `shortRecord_1.flaggerId` & `shortRecord_2.flaggerId` are 1 now. This
// means that they are now mapped to the same address - that of flagger2. This is mapped via `s.flagMapping[short.flaggerId] => flagger_address`.
// bug: Flagger1 can now not liquidate `shortRecord_1` even if he tries. However, flagger2 can now liquidate `shortRecord_1`, and
// receive the proceeds, violating flagger1's priority liquidation time-window. This is despite the fact that flagger2 had never flagged short1.
// !!
fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver); // adding an ask order to avoid `Errors.NoSells()` during liquidation
// pre-verfication:
assertEq(
diamond.getVaultUserStruct(vault, _flagger_02_lucky_fella).ethEscrowed, 0
);
assertEq(diamond.getVaultUserStruct(vault, _flagger_01).ethEscrowed, 0);
​
// flagger1 is unable to liquidate `shortRecord_1` now
vm.prank(_flagger_01);
vm.expectRevert(Errors.MarginCallIneligibleWindow.selector);
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
​
// flagger2 has "received" the liquidation rights now for `shortRecord_1` now!
vm.prank(_flagger_02_lucky_fella);
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
​
// post-verfication:
// verify -
// - a. if short1 has been liquidated (shortRecord id is no more valid)
// - b. if flagger2 is rewarded
// - c. that flagger1 is not rewarded
​
// a
vm.prank(_flagger_01);
vm.expectRevert(Errors.InvalidShortId.selector);
diamond.liquidate(
asset, sender, Constants.SHORT_STARTING_ID, shortHintArrayStorage
);
​
// b
assertGt(
diamond.getVaultUserStruct(vault, _flagger_02_lucky_fella).ethEscrowed,
0 ether,
"flagger2 not rewarded"
);
​
// c
assertEq(
diamond.getVaultUserStruct(vault, _flagger_01).ethEscrowed,
0 ether,
"flagger1 got rewarded"
);
}
}

Tools Used

Manual review, foundry.

Recommendations

File: contracts/libraries/LibShortRecord.sol
​
377 function setFlagger(
378 STypes.ShortRecord storage short,
379 address cusd,
380 uint16 flaggerHint
381 ) internal {
382 AppStorage storage s = appStorage();
383 STypes.AssetUser storage flagStorage = s.assetUser[cusd][msg.sender];
384
385 //@dev Whenever a new flagger flags, use the flaggerIdCounter.
386 if (flagStorage.g_flaggerId == 0) {
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)) {
+ 394 if (timeDiff > LibAsset.secondLiquidationTime(cusd)) {
395 delete s.assetUser[cusd][flaggerToReplace].g_flaggerId;
396 short.flaggerId = flagStorage.g_flaggerId = flaggerHint;
397 } else if (s.flaggerIdCounter < type(uint16).max) {
398 //@dev generate brand new flaggerId
399 short.flaggerId = flagStorage.g_flaggerId = s.flaggerIdCounter;
400 s.flaggerIdCounter++;
401 } else {
402 revert Errors.InvalidFlaggerHint();
403 }
404 s.flagMapping[short.flaggerId] = msg.sender;
405 } else {
406 //@dev re-use flaggerId if flagger has an existing one
407 short.flaggerId = flagStorage.g_flaggerId;
408 }
409 short.updatedAt = flagStorage.g_updatedAt = LibOrders.getOffsetTimeHours();
410 }
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.