Submission Details
Severity:
Wrong logic in LibShortRecord#setShortRecordIds
Summary
Wrong logic in LibShortRecord#setShortRecordIds
Vulnerability Details
A new short record can reuse the existing cancel ID.
After reusing, the prev ID is set to prevCanceledId (See LibShortRecord.sol#L254).
guard.prevId = prevCanceledId;
This is incorrect. Looking at the representation below
// BEFORE: CancelledID <- (ID) <- HEAD <-> .. <-> PREV <----------> NEXT
// AFTER1: CancelledID <--------- HEAD <-> .. <-> PREV <-> [ID] <-> NEXT
After reusing the cancel ID, the prev should be set to head
Impact
The short linked list would not linked correctly
Tools Used
Manual
Recommendations
Change LibShortRecord#setShortRecordIds
function setShortRecordIds(address asset, address shorter)
private
returns (uint8 id, uint8 nextId)
{
AppStorage storage s = appStorage();
STypes.ShortRecord storage guard = s.shortRecords[asset][shorter][Constants.HEAD];
STypes.AssetUser storage AssetUser = s.assetUser[asset][shorter];
// Initialize HEAD in case of first short createShortRecord
if (AssetUser.shortRecordId == 0) {
AssetUser.shortRecordId = Constants.SHORT_STARTING_ID;
guard.prevId = Constants.HEAD;
guard.nextId = Constants.HEAD;
}
// BEFORE: HEAD <-> .. <-> PREV <--------------> NEXT
// AFTER1: HEAD <-> .. <-> PREV <-> (NEW ID) <-> NEXT
// place created short next to HEAD
nextId = guard.nextId;
uint8 canceledId = guard.prevId;
// @dev (ID) is exiting, [ID] is inserted
// in this case, the protocol re-uses (ID) and moves it to [ID]
// check if a previously closed short exists
if (canceledId > Constants.HEAD) {
// BEFORE: CancelledID <- (ID) <- HEAD <-> .. <-> PREV <----------> NEXT
// AFTER1: CancelledID <--------- HEAD <-> .. <-> PREV <-> [ID] <-> NEXT
uint8 prevCanceledId = s.shortRecords[asset][shorter][canceledId].prevId;
- if (prevCanceledId > Constants.HEAD) {
- guard.prevId = prevCanceledId;
- } else {
- // BEFORE: HEAD <- (ID) <- HEAD <-> .. <-> PREV <----------> NEXT
- // AFTER1: HEAD <--------- HEAD <-> .. <-> PREV <-> [ID] <-> NEXT
- guard.prevId = Constants.HEAD;
- }
+ guard.prevId = Constants.HEAD;
// re-use the previous order's id
id = canceledId;
} else {
// BEFORE: HEAD <-> .. <-> PREV <--------------> NEXT
// AFTER1: HEAD <-> .. <-> PREV <-> (NEW ID) <-> NEXT
// otherwise just increment to a new short record id
// and the short record grows in height/size
id = AssetUser.shortRecordId;
// Avoids overflow revert, prevents DOS on uint8
if (id < type(uint8).max) {
AssetUser.shortRecordId += 1;
} else {
// If max id reached, match into max shortRecordId
return (id, nextId);
}
}
if (nextId > Constants.HEAD) {
s.shortRecords[asset][shorter][nextId].prevId = id;
}
guard.nextId = id;
}
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.