DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Liquidating a partially filled short record leaves it linked in the wrong section of the short records linked list of a trader

flacko

Summary

Liquidating an unhealthy short record through the MarginCallSecondaryFacet will not update it's position and links in the shortRecords linked list of the trader or the TAPP in storage.

Vulnerability Details

Whenever a short record is liquidated in through the MarginCallSecondaryFacet contract, it's being covered fully by the caller of liquidateSecondary() if they have enough balance of the asset or partially by the TAPP if they specified so:

function liquidateSecondary(
address asset,
MTypes.BatchMC[] memory batches,
uint88 liquidateAmount,
bool isWallet
) external onlyValidAsset(asset) isNotFrozen(asset) nonReentrant {
// ... code skipped for brevity
​
bool partialTappLiquidation;
// Setup partial liquidation of TAPP short
if (state.shorter == address(this)) {
partialTappLiquidation = liquidateAmountLeft < state.short.ercDebt;
if (partialTappLiquidation) {
state.short.ercDebt = liquidateAmountLeft;
}
}
​
if (isWallet) {
IAsset tokenContract = IAsset(asset);
uint256 walletBalance = tokenContract.balanceOf(msg.sender);
if (walletBalance < m.short.ercDebt) continue;
tokenContract.burnFrom(msg.sender, m.short.ercDebt);
assert(tokenContract.balanceOf(msg.sender) < walletBalance);
} else {
if (AssetUser.ercEscrowed < m.short.ercDebt) {
continue;
}
AssetUser.ercEscrowed -= m.short.ercDebt;
}
// ... code skipped for brevity
}

Right after deducting the ERC asset from the caller's wallet or escrow, the code proceeds to call either _secondaryLiquidationHelperPartialTapp if the short record belongs to the TAPP and it's being liquidated partially, or it calls _secondaryLiquidationHelper otherwise.

if (partialTappLiquidation) {
// Partial liquidation of TAPP short
_secondaryLiquidationHelperPartialTapp(m);
} else {
// Full liquidation
_secondaryLiquidationHelper(m);
}

Looking at what _secondaryLiquidationHelper() does we see that it massages ETH escrow balances, calls disburseCollateral() and deleteShortRecord().

// Note: Code squashed for brevity
​
function _secondaryLiquidationHelper(MTypes.MarginCallSecondary memory m) private {
m.liquidatorCollateral = m.short.collateral;
if (m.cRatio > 1 ether) {
uint88 ercDebtAtOraclePrice =
m.short.ercDebt.mulU88(LibOracle.getPrice(m.asset)); // eth
m.liquidatorCollateral = ercDebtAtOraclePrice;
address remainingCollateralAddress =
m.cRatio > m.minimumCR ? m.shorter : address(this);
​
s.vaultUser[m.vault][remainingCollateralAddress].ethEscrowed +=
m.short.collateral - ercDebtAtOraclePrice;
}
LibShortRecord.disburseCollateral(
m.asset,
m.shorter,
m.short.collateral,
m.short.zethYieldRate,
m.short.updatedAt
);
LibShortRecord.deleteShortRecord(m.asset, m.shorter, m.short.id);
}

The only method here that takes care of the position of the now fully liquidated short record in the shortRecords linked list is the call to LibShortRecord.deleteShortRecord().

function deleteShortRecord(address asset, address shorter, uint8 id) internal {
AppStorage storage s = appStorage();
STypes.ShortRecord storage shortRecord = s.shortRecords[asset][shorter][id];
if (shortRecord.status != SR.PartialFill && id < Constants.SHORT_MAX_ID) {
s.shortRecords[asset][shorter][shortRecord.prevId].nextId = shortRecord.nextId;
if (shortRecord.nextId != Constants.HEAD) {
s.shortRecords[asset][shorter][shortRecord.nextId].prevId =
shortRecord.prevId;
}
uint8 prevHEAD = s.shortRecords[asset][shorter][Constants.HEAD].prevId;
s.shortRecords[asset][shorter][Constants.HEAD].prevId = id;
if (prevHEAD > Constants.HEAD) {
shortRecord.prevId = prevHEAD;
} else {
shortRecord.prevId = Constants.HEAD;
}
emit Events.DeleteShortRecord(asset, shorter, id);
}
​
shortRecord.status = SR.Cancelled;
}

What can be seen here is that a short record is moved to the cancelled short records part of the linked list (behind the HEAD pointer) only if it's status is not PartialFill (and it's id is < than the max short id [254], which will be irrelevant for the first 253 short records).

Impact

Because the short record is not moved to its proper place in the shortRecords linked list (pointing back to HEAD) of a trader or the TAPP, that linked list will occasionally get polluted with liquidated short records.

Tools Used

Manual review

Recommendations

Upon liquidation, change the status of the short record to FullyFilled right before _secondaryLiquidationHelper() so that LibShortRecord.deleteShortRecord() can take proper care of the short record and update the shortRecords linked list accordingly.

Updates

Lead Judging Commences

0xnevi Lead Judge
almost 2 years ago
0xnevi Lead Judge
almost 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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