Submission Details
Severity:
User's can loose their combined collateral if first id is passed twice in `combineShorts`
Vulnerability Details
When combining short orders, the ids array is not checked for duplication nor is the current status of the first short checked right before the merge or inside the merge function itself.
function combineShorts(address asset, uint8[] memory ids)
external
isNotFrozen(asset)
nonReentrant
onlyValidShortRecord(asset, msg.sender, ids[0])
{
// more code
// @audit id is not checked for duplication of first id.
for (uint256 i = ids.length - 1; i > 0; i--) {
uint8 _id = ids[i];
_onlyValidShortRecord(_asset, msg.sender, _id);
// more code
// @audit current status of firstShort not checked
// Merge all short records into the short at position id[0]
firstShort.merge(ercDebt, ercDebtSocialized, collateral, yield, c.shortUpdatedAt);
https://github.com/Cyfrin/2023-09-ditto/blob/a93b4276420a092913f43169a353a6198d3c21b9/contracts/facets/ShortRecordFacet.sol#L117C14-L176
Hence the firstId could repeat in the ids array, which will cause the firstShort to be marked cancelled inside the loop but still proceed to merge with the firstShort.
POC Test
diff --git a/test/Shorts.t.sol b/test/Shorts.t.sol
index f1c3927..88ef28a 100644
--- a/test/Shorts.t.sol
+++ b/test/Shorts.t.sol
contract ShortsTest is OBFixture {
);
}
+ function test_CombiningReapeatedFirstLeadsToLoss() public {
+ fundLimitBidOpt(SHORT1_PRICE, DEFAULT_AMOUNT, receiver);
+ fundLimitShortOpt(SHORT1_PRICE, DEFAULT_AMOUNT, sender);
+ STypes.ShortRecord memory shortRecord1BeforeCombine =
+ getShortRecord(sender, Constants.SHORT_STARTING_ID);
+
+ fundLimitBidOpt(SHORT1_PRICE, DEFAULT_AMOUNT, receiver);
+ fundLimitShortOpt(SHORT1_PRICE, DEFAULT_AMOUNT, sender);
+ STypes.ShortRecord memory shortRecord2BeforeCombine =
+ getShortRecord(sender, Constants.SHORT_STARTING_ID + 1);
+
+ //providing the 1st short twice in the array
+ vm.prank(sender);
+ uint8[] memory shortRecords = new uint8[](3);
+ shortRecords[0] = shortRecord1BeforeCombine.id;
+ shortRecords[1] = shortRecord1BeforeCombine.id;
+ shortRecords[2] = shortRecord2BeforeCombine.id;
+ diamond.combineShorts(asset, shortRecords);
+
+ STypes.ShortRecord memory shortRecord1AfterCombine =
+ getShortRecord(sender, shortRecord1BeforeCombine.id);
+ STypes.ShortRecord memory shortRecord2AfterCombine =
+ getShortRecord(sender, shortRecord2BeforeCombine.id);
+
+ //the short records are cancelled and the entire collateral is in the cancelled shortRecord1
+ assertEq(shortRecord1AfterCombine.status == SR.Cancelled, true);
+ assertEq(shortRecord2AfterCombine.status == SR.Cancelled, true);
+ assertEq(
+ shortRecord1AfterCombine.collateral,
+ shortRecord1BeforeCombine.collateral + shortRecord1BeforeCombine.collateral + shortRecord2BeforeCombine.collateral
+ );
+ }
+
//////Combine Shorts//////
function testCombineShortsx2() public {
makeShorts();
Impact
Loss of user's collateral
Recommendations
Validate if the firstId repeats in the array or check the status of the firstShort just before merge.
diff --git a/contracts/facets/ShortRecordFacet.sol b/contracts/facets/ShortRecordFacet.sol
index 3fe7e18..44058a6 100644
--- a/contracts/facets/ShortRecordFacet.sol
+++ b/contracts/facets/ShortRecordFacet.sol
import {U256, U80, U88} from "contracts/libraries/PRBMathHelper.sol";
import {Errors} from "contracts/libraries/Errors.sol";
import {Events} from "contracts/libraries/Events.sol";
import {Modifiers} from "contracts/libraries/AppStorage.sol";
-import {STypes, MTypes} from "contracts/libraries/DataTypes.sol";
+import {STypes, SR, MTypes} from "contracts/libraries/DataTypes.sol";
import {LibAsset} from "contracts/libraries/LibAsset.sol";
import {LibShortRecord} from "contracts/libraries/LibShortRecord.sol";
import {LibOracle} from "contracts/libraries/LibOracle.sol";
contract ShortRecordFacet is Modifiers {
LibShortRecord.deleteShortRecord(_asset, msg.sender, _id);
}
+ //check for cancellation
+ if (firstShort.status == SR.Cancelled) {
+ revert("Duplicate First Short Id");
+ }
// Merge all short records into the short at position id[0]
firstShort.merge(ercDebt, ercDebtSocialized, collateral, yield, c.shortUpdatedAt);
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.