Submission Details
Severity:
Users can inadvertently delete their short records via `combineShorts()` and lose all collateral
Summary
Bug in the logic of combineShorts() can easily cause users to delete one or all of their short records. This causes their collateral and ercDebt to become 0 (they lose all their collateral).
Vulnerability Details
The happy path of combineShorts() expects a user to call it in a manner resembling the following (Note: Constants.SHORT_STARTING_ID has a value 2):
uint8[] memory shortIds_to_combine = new uint8[](3);
shortIds_to_combine[0] = Constants.SHORT_STARTING_ID + 2; // id: 4
shortIds_to_combine[1] = Constants.SHORT_STARTING_ID + 1; // id: 3
shortIds_to_combine[2] = Constants.SHORT_STARTING_ID; // id: 2
vm.prank(receiver);
diamond.combineShorts(_cusd, shortIds_to_combine);
This merges short records with ids 2 and 3 into id 4. Short record ids 2 and 3 are not needed any more, so they are deleted.
Even if a user provides an array with duplicate elements in index 1 to n, it doesn't break the system as the protocol rightly reverts with an `InvalidShortId()` error. So, the following will revert:
uint8[] memory shortIds_to_combine = new uint8[](3);
shortIds_to_combine[0] = Constants.SHORT_STARTING_ID + 2;
shortIds_to_combine[1] = Constants.SHORT_STARTING_ID + 1;
shortIds_to_combine[2] = Constants.SHORT_STARTING_ID + 1; // @audit-info : id can not be the same as above
vm.prank(receiver);
diamond.combineShorts(_cusd, shortIds_to_combine);
However, if a user provides the same short record id in shortIds_to_combine[n] as in shortIds_to_combine[0], then all the short records provided in the array are deleted, even the one they merged into. For example:
uint8[] memory shortIds_to_combine = new uint8[](4);
shortIds_to_combine[0] = Constants.SHORT_STARTING_ID + 2;
shortIds_to_combine[1] = Constants.SHORT_STARTING_ID + 1;
shortIds_to_combine[2] = Constants.SHORT_STARTING_ID;
shortIds_to_combine[3] = Constants.SHORT_STARTING_ID + 2; // @audit-issue : since this id is the same as the one provided at index 0, this will cause short records with ids 2, 3, 4 to be deleted altogether.
vm.prank(receiver);
diamond.combineShorts(_cusd, shortIds_to_combine);
Since the short record does not exist anymore, this causes its collateral and ercDebt to become 0 (or non-existent) causing loss of funds for the user.
PoC
Add the following code inside test/fork/EndToEndFork.t.sol and run it through forge test --mt testFork_shortRecordCanBeDeletedViaCombineShorts -vv. Remember to uncomment L11 to enable logging:
/* solhint-disable no-console */
function testFork_shortRecordCanBeDeletedViaCombineShorts() public {
uint16 cusdInitialMargin = diamond.getAssetStruct(_cusd).initialMargin; // 200%
// Workflow: Bridge - DepositEth
// sender
vm.startPrank(sender);
assertEq(reth.balanceOf(_bridgeReth), 0);
diamond.depositEth{value: 500 ether}(_bridgeReth);
vm.stopPrank();
// receiver
vm.startPrank(receiver);
assertEq(steth.balanceOf(_bridgeSteth), 0);
diamond.depositEth{value: 500 ether}(_bridgeSteth);
// Workflow: LimitShort
MTypes.OrderHint[] memory orderHints = new MTypes.OrderHint[](1);
currentPrice = diamond.getOraclePriceT(_cusd); // ~ 0.0005 ether
diamond.createLimitShort(
_cusd, currentPrice, 100_000 ether, orderHints, shortHints, cusdInitialMargin
);
vm.stopPrank();
// Workflow: LimitBid
// sender
shortHints[0] = diamond.getShortIdAtOracle(_cusd);
vm.prank(sender);
diamond.createBid(
_cusd, currentPrice, 100_000 ether, false, orderHints, shortHints
);
// @audit-info : short record with id: 2 would have been created here. Let's fetch it.
STypes.ShortRecord memory receiverShort =
diamond.getShortRecords(_cusd, receiver)[0];
assertEq(receiverShort.ercDebt, 100_000 ether);
assertSR(SR.FullyFilled, receiverShort.status);
console.log(
"total receiver collateral here",
diamond.getShortRecords(_cusd, receiver)[0].collateral
); // ~150 ether
vm.startPrank(receiver);
// removing 50 ether as minimum collateral required to be maintained is 100 ether
diamond.decreaseCollateral(_cusd, 2, 50 ether);
console.log("=================== BEFORE combine ==========================");
console.log(
"receiver collateral", diamond.getShortRecords(_cusd, receiver)[0].collateral
);
console.log(
"Number of short records", (diamond.getShortRecords(_cusd, receiver)).length
);
console.log("=========================================================");
// @audit-info : let's combine(delete) the short
uint8[] memory shortIds_to_combine = new uint8[](2);
shortIds_to_combine[0] = Constants.SHORT_STARTING_ID;
// @audit-issue : This is the root cause of the bug. Keeping last index same as the first one deletes the whole merged short record
shortIds_to_combine[1] = Constants.SHORT_STARTING_ID;
diamond.combineShorts(_cusd, shortIds_to_combine); // @audit : This will "combine" and also delete the short record id 2
console.log("=================== AFTER combine ==========================");
console.log(
"receiver shortRecords count after buggy combine/deletion",
(diamond.getShortRecords(_cusd, receiver)).length
); // 0
console.log(
"receiver collateral all gone from shortRecord diamond.getShortRecords(_cusd, receiver)[0].collateral since shortRecord is deleted."
);
vm.stopPrank();
console.log("=========================================================");
// Ideally, combineShorts() should have resulted in 1 final shortRecord being present, but
// that is not the case.
assertGt(
(diamond.getShortRecords(_cusd, receiver)).length,
0,
"All Short Records Deleted"
);
}
The output is the following:
Logs:
total receiver collateral here 150549361698162000000
=================== BEFORE combine ==========================
receiver collateral 100549361698162000000
Number of short records 1
=========================================================
=================== AFTER combine ==========================
receiver shortRecords count after buggy combine/deletion 0
receiver collateral all gone from shortRecord diamond.getShortRecords(_cusd, receiver)[0].collateral since shortRecord is deleted.
=========================================================
Error: All Short Records Deleted
Error: a > b not satisfied [uint]
Value a: 0
Value b: 0
Impact
User loses all of her collateral. If we assume this to be an honest mistake by the user where she passes the same id twice, then the penalty she has to pay for this is huge - losing all her collateral across all the short records she passed in the array.
Direct deletion of short record possible without going through the flow of liquidation/exit first.
Tools Used
Manual review, forge test.
Recommendations
Inside combineShorts() add a check that the last id in the array ids is not the same as that at the first index.
File: contracts/facets/ShortRecordFacet.sol
117 function combineShorts(address asset, uint8[] memory ids)
118 external
119 isNotFrozen(asset)
120 nonReentrant
121 onlyValidShortRecord(asset, msg.sender, ids[0])
122 {
123 if (ids.length < 2) revert Errors.InsufficientNumberOfShorts();
+ require(ids[ids.length - 1] != ids[0]);
124 // First short in the array
125 STypes.ShortRecord storage firstShort = s.shortRecords[asset][msg.sender][ids[0]];
126 // @dev Load initial short elements in struct to avoid stack too deep
127 MTypes.CombineShorts memory c;
128 c.shortFlagExists = firstShort.flaggerId != 0;
129 c.shortUpdatedAt = firstShort.updatedAt;
130
131 address _asset = asset;
132 uint88 collateral;
133 uint88 ercDebt;
134 uint256 yield;
135 uint256 ercDebtSocialized;
136 for (uint256 i = ids.length - 1; i > 0; i--) {
137 uint8 _id = ids[i];
138 _onlyValidShortRecord(_asset, msg.sender, _id);
139 STypes.ShortRecord storage currentShort =
140 s.shortRecords[_asset][msg.sender][_id];
141 // See if there is at least one flagged short
142 if (!c.shortFlagExists) {
143 if (currentShort.flaggerId != 0) {
144 c.shortFlagExists = true;
145 }
146 }
147
148 //@dev Take latest time when combining shorts (prevent flash loan)
149 if (currentShort.updatedAt > c.shortUpdatedAt) {
150 c.shortUpdatedAt = currentShort.updatedAt;
151 }
152
153 {
154 uint88 currentShortCollateral = currentShort.collateral;
155 uint88 currentShortErcDebt = currentShort.ercDebt;
156 collateral += currentShortCollateral;
157 ercDebt += currentShortErcDebt;
158 yield += currentShortCollateral.mul(currentShort.zethYieldRate);
159 ercDebtSocialized += currentShortErcDebt.mul(currentShort.ercDebtRate);
160 }
161
162 if (currentShort.tokenId != 0) {
163 //@dev First short needs to have NFT so there isn't a need to burn and re-mint
164 if (firstShort.tokenId == 0) {
165 revert Errors.FirstShortMustBeNFT();
166 }
167
168 LibShortRecord.burnNFT(currentShort.tokenId);
169 }
170
171 // Cancel this short and combine with short in ids[0]
172 LibShortRecord.deleteShortRecord(_asset, msg.sender, _id);
173 }
174
175 // Merge all short records into the short at position id[0]
176 firstShort.merge(ercDebt, ercDebtSocialized, collateral, yield, c.shortUpdatedAt);
177
178 // If at least one short was flagged, ensure resulting c-ratio > primaryLiquidationCR
179 if (c.shortFlagExists) {
180 if (
181 firstShort.getCollateralRatioSpotPrice(
182 LibOracle.getSavedOrSpotOraclePrice(_asset)
183 ) < LibAsset.primaryLiquidationCR(_asset)
184 ) revert Errors.InsufficientCollateral();
185 // Resulting combined short has sufficient c-ratio to remove flag
186 firstShort.resetFlag();
187 }
188 emit Events.CombineShorts(asset, msg.sender, ids);
189 }
Prize pool breakdown
Total prize
55,000 USDC
nSLOC
3,36516
USDC / LOC
50,000 USDC
5,000 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.