-
Notifications
You must be signed in to change notification settings - Fork 271
feat(orc-537): Update hash consesnsus contract to prevent attack on _… #1580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 2 commits
34f4055
57203bf
784084d
91851f7
b9320c7
8f4de2b
b53f44f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -149,23 +149,15 @@ | |||||||||||||||
| uint64 lastReportRefSlot; | ||||||||||||||||
| // the last reference slot a consensus was reached for | ||||||||||||||||
| uint64 lastConsensusRefSlot; | ||||||||||||||||
| // the last consensus variant index | ||||||||||||||||
| uint64 lastConsensusVariantIndex; | ||||||||||||||||
| // the last consensus report hash | ||||||||||||||||
| bytes32 lastConsensusReport; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| struct MemberState { | ||||||||||||||||
| // the last reference slot a report from this member was received for | ||||||||||||||||
| uint64 lastReportRefSlot; | ||||||||||||||||
| // the variant index of the last report from this member | ||||||||||||||||
| uint64 lastReportVariantIndex; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| struct ReportVariant { | ||||||||||||||||
| // the reported hash | ||||||||||||||||
| bytes32 hash; | ||||||||||||||||
| // how many unique members from the current set reported this hash in the current frame | ||||||||||||||||
| uint64 support; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /// @notice An ACL role granting the permission to modify members list members and | ||||||||||||||||
| /// change the quorum by calling addMember, removeMember, and setQuorum functions. | ||||||||||||||||
|
|
@@ -224,11 +216,14 @@ | |||||||||||||||
| /// @dev Oracle committee members quorum value, must be larger than totalMembers // 2 | ||||||||||||||||
| uint256 internal _quorum; | ||||||||||||||||
|
|
||||||||||||||||
| /// @dev Mapping from a report variant index to the ReportVariant structure | ||||||||||||||||
| mapping(uint256 => ReportVariant) internal _reportVariants; | ||||||||||||||||
| /// @dev Mapping from member address to their current report hash for the current frame | ||||||||||||||||
| mapping(address => bytes32) internal _memberReports; | ||||||||||||||||
|
|
||||||||||||||||
| /// @dev Mapping from report hash to the number of members supporting it | ||||||||||||||||
| mapping(bytes32 => uint256) internal _hashSupport; | ||||||||||||||||
|
|
||||||||||||||||
| /// @dev The number of report variants | ||||||||||||||||
| uint256 internal _reportVariantsLength; | ||||||||||||||||
| /// @dev Array of addresses that submitted reports in the current frame | ||||||||||||||||
| address[] internal _currentFrameReporters; | ||||||||||||||||
|
|
||||||||||||||||
| /// @dev The address of the report processor contract | ||||||||||||||||
| address internal _reportProcessor; | ||||||||||||||||
|
|
@@ -503,7 +498,7 @@ | |||||||||||||||
| bool isReportProcessing | ||||||||||||||||
| ) { | ||||||||||||||||
| refSlot = _getCurrentFrame().refSlot; | ||||||||||||||||
| (consensusReport,,) = _getConsensusReport(refSlot, _quorum); | ||||||||||||||||
| (consensusReport,) = _getConsensusReport(refSlot, _quorum); | ||||||||||||||||
| isReportProcessing = _getLastProcessingRefSlot() == refSlot; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -517,15 +512,32 @@ | |||||||||||||||
| return (variants, support); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| uint256 variantsLength = _reportVariantsLength; | ||||||||||||||||
| variants = new bytes32[](variantsLength); | ||||||||||||||||
| support = new uint256[](variantsLength); | ||||||||||||||||
|
|
||||||||||||||||
| for (uint256 i = 0; i < variantsLength; ++i) { | ||||||||||||||||
| ReportVariant memory variant = _reportVariants[i]; | ||||||||||||||||
| variants[i] = variant.hash; | ||||||||||||||||
| support[i] = variant.support; | ||||||||||||||||
| // Collect unique hashes and their support from member reports in one pass | ||||||||||||||||
| address[] memory members = _memberAddresses; | ||||||||||||||||
| variants = new bytes32[](members.length); | ||||||||||||||||
| support = new uint256[](members.length); | ||||||||||||||||
|
|
||||||||||||||||
| uint256 variantCount = 0; | ||||||||||||||||
|
|
||||||||||||||||
| for (uint256 i = 0; i < members.length; i++) { | ||||||||||||||||
| bytes32 memberHash = _memberReports[members[i]]; | ||||||||||||||||
| if (memberHash != ZERO_HASH) { | ||||||||||||||||
| bool found = false; | ||||||||||||||||
| for (uint256 j = 0; j < variantCount; j++) { | ||||||||||||||||
| if (variants[j] == memberHash) { | ||||||||||||||||
| found = true; | ||||||||||||||||
| break; | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| if (!found) { | ||||||||||||||||
| variants[variantCount] = memberHash; | ||||||||||||||||
| support[variantCount] = _hashSupport[memberHash]; | ||||||||||||||||
| variantCount++; | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| return (variants, support); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| struct MemberConsensusState { | ||||||||||||||||
|
|
@@ -566,7 +578,7 @@ | |||||||||||||||
| { | ||||||||||||||||
| ConsensusFrame memory frame = _getCurrentFrame(); | ||||||||||||||||
| result.currentFrameRefSlot = frame.refSlot; | ||||||||||||||||
| (result.currentFrameConsensusReport,,) = _getConsensusReport(frame.refSlot, _quorum); | ||||||||||||||||
| (result.currentFrameConsensusReport,) = _getConsensusReport(frame.refSlot, _quorum); | ||||||||||||||||
|
|
||||||||||||||||
| uint256 index = _memberIndices1b[addr]; | ||||||||||||||||
| result.isMember = index != 0; | ||||||||||||||||
|
|
@@ -578,7 +590,7 @@ | |||||||||||||||
| result.lastMemberReportRefSlot = memberState.lastReportRefSlot; | ||||||||||||||||
| result.currentFrameMemberReport = | ||||||||||||||||
| result.lastMemberReportRefSlot == frame.refSlot | ||||||||||||||||
| ? _reportVariants[memberState.lastReportVariantIndex].hash | ||||||||||||||||
| ? _memberReports[addr] | ||||||||||||||||
| : ZERO_HASH; | ||||||||||||||||
|
|
||||||||||||||||
| uint256 slot = _computeSlotAtTimestamp(_getTime()); | ||||||||||||||||
|
|
@@ -740,7 +752,7 @@ | |||||||||||||||
| if (_isMember(addr)) revert DuplicateMember(); | ||||||||||||||||
| if (addr == address(0)) revert AddressCannotBeZero(); | ||||||||||||||||
|
|
||||||||||||||||
| _memberStates.push(MemberState(0, 0)); | ||||||||||||||||
| _memberStates.push(MemberState(0)); | ||||||||||||||||
| _memberAddresses.push(addr); | ||||||||||||||||
|
|
||||||||||||||||
| uint256 newTotalMembers = _memberStates.length; | ||||||||||||||||
|
|
@@ -780,7 +792,15 @@ | |||||||||||||||
| ) { | ||||||||||||||||
| // member reported for the current ref. slot and the consensus report | ||||||||||||||||
| // is not processing yet => need to cancel the member's report | ||||||||||||||||
| --_reportVariants[memberState.lastReportVariantIndex].support; | ||||||||||||||||
| bytes32 memberHash = _memberReports[addr]; | ||||||||||||||||
| if (memberHash != ZERO_HASH) { | ||||||||||||||||
| if (_hashSupport[memberHash] <= 1) { | ||||||||||||||||
| delete _hashSupport[memberHash]; | ||||||||||||||||
| } else { | ||||||||||||||||
| --_hashSupport[memberHash]; | ||||||||||||||||
| } | ||||||||||||||||
| delete _memberReports[addr]; | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -895,55 +915,54 @@ | |||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| uint256 variantsLength; | ||||||||||||||||
|
|
||||||||||||||||
| if (_reportingState.lastReportRefSlot != slot) { | ||||||||||||||||
| // first report for a new slot => clear report variants | ||||||||||||||||
| // first report for a new slot => clear report mappings for new frame | ||||||||||||||||
| _reportingState.lastReportRefSlot = uint64(slot); | ||||||||||||||||
| variantsLength = 0; | ||||||||||||||||
| } else { | ||||||||||||||||
| variantsLength = _reportVariantsLength; | ||||||||||||||||
| _clearMemberReports(); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| uint64 varIndex = 0; | ||||||||||||||||
| address member = _msgSender(); | ||||||||||||||||
| bytes32 oldReport = _memberReports[member]; | ||||||||||||||||
| bool prevConsensusLost = false; | ||||||||||||||||
|
|
||||||||||||||||
| while (varIndex < variantsLength && _reportVariants[varIndex].hash != report) { | ||||||||||||||||
| ++varIndex; | ||||||||||||||||
| if (oldReport == report) { | ||||||||||||||||
| revert DuplicateReport(); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if (slot == memberState.lastReportRefSlot) { | ||||||||||||||||
| uint64 prevVarIndex = memberState.lastReportVariantIndex; | ||||||||||||||||
| assert(prevVarIndex < variantsLength); | ||||||||||||||||
| if (varIndex == prevVarIndex) { | ||||||||||||||||
| revert DuplicateReport(); | ||||||||||||||||
| // Remove support from member's previous report (if any) | ||||||||||||||||
| if (oldReport != ZERO_HASH) { | ||||||||||||||||
| uint256 oldSupport = _hashSupport[oldReport]; | ||||||||||||||||
|
|
||||||||||||||||
| if (oldSupport <= 1) { | ||||||||||||||||
| delete _hashSupport[oldReport]; | ||||||||||||||||
| oldSupport = 0; | ||||||||||||||||
| } else { | ||||||||||||||||
| uint256 support = --_reportVariants[prevVarIndex].support; | ||||||||||||||||
| if (support == _quorum - 1) { | ||||||||||||||||
| prevConsensusLost = true; | ||||||||||||||||
| } | ||||||||||||||||
| --_hashSupport[oldReport]; | ||||||||||||||||
| oldSupport--; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if (oldSupport == _quorum - 1) { | ||||||||||||||||
| prevConsensusLost = true; | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| uint256 support; | ||||||||||||||||
| // Add support to the new report | ||||||||||||||||
| _memberReports[member] = report; | ||||||||||||||||
| uint256 support = ++_hashSupport[report]; | ||||||||||||||||
|
|
||||||||||||||||
| if (varIndex < variantsLength) { | ||||||||||||||||
| support = ++_reportVariants[varIndex].support; | ||||||||||||||||
| } else { | ||||||||||||||||
| support = 1; | ||||||||||||||||
| _reportVariants[varIndex] = ReportVariant({hash: report, support: 1}); | ||||||||||||||||
| _reportVariantsLength = ++variantsLength; | ||||||||||||||||
| // Track this member as a reporter in current frame (avoid duplicates) | ||||||||||||||||
| if (oldReport == ZERO_HASH) { | ||||||||||||||||
| _currentFrameReporters.push(member); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| _memberStates[memberIndex] = MemberState({ | ||||||||||||||||
| lastReportRefSlot: uint64(slot), | ||||||||||||||||
| lastReportVariantIndex: varIndex | ||||||||||||||||
| lastReportRefSlot: uint64(slot) | ||||||||||||||||
| }); | ||||||||||||||||
|
|
||||||||||||||||
| emit ReportReceived(slot, _msgSender(), report); | ||||||||||||||||
|
|
||||||||||||||||
| if (support >= _quorum) { | ||||||||||||||||
| _consensusReached(frame, report, varIndex, support); | ||||||||||||||||
| _consensusReached(frame, report, support); | ||||||||||||||||
| } else if (prevConsensusLost) { | ||||||||||||||||
| _consensusNotReached(frame); | ||||||||||||||||
| } | ||||||||||||||||
|
|
@@ -952,14 +971,13 @@ | |||||||||||||||
| function _consensusReached( | ||||||||||||||||
| ConsensusFrame memory frame, | ||||||||||||||||
| bytes32 report, | ||||||||||||||||
| uint256 variantIndex, | ||||||||||||||||
| uint256 support | ||||||||||||||||
| ) internal { | ||||||||||||||||
| if (_reportingState.lastConsensusRefSlot != frame.refSlot || | ||||||||||||||||
| _reportingState.lastConsensusVariantIndex != variantIndex | ||||||||||||||||
| _reportingState.lastConsensusReport != report | ||||||||||||||||
| ) { | ||||||||||||||||
| _reportingState.lastConsensusRefSlot = uint64(frame.refSlot); | ||||||||||||||||
| _reportingState.lastConsensusVariantIndex = uint64(variantIndex); | ||||||||||||||||
| _reportingState.lastConsensusReport = report; | ||||||||||||||||
| emit ConsensusReached(frame.refSlot, report, support); | ||||||||||||||||
| _submitReportForProcessing(frame, report); | ||||||||||||||||
| } | ||||||||||||||||
|
|
@@ -1010,40 +1028,37 @@ | |||||||||||||||
| return; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| (bytes32 consensusReport, int256 consensusVariantIndex, uint256 support) = | ||||||||||||||||
| (bytes32 consensusReport, uint256 support) = | ||||||||||||||||
| _getConsensusReport(frame.refSlot, quorum); | ||||||||||||||||
|
|
||||||||||||||||
| if (consensusVariantIndex >= 0) { | ||||||||||||||||
| _consensusReached(frame, consensusReport, uint256(consensusVariantIndex), support); | ||||||||||||||||
| if (consensusReport != ZERO_HASH && support >= quorum) { | ||||||||||||||||
| _consensusReached(frame, consensusReport, support); | ||||||||||||||||
| } else { | ||||||||||||||||
| _consensusNotReached(frame); | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| function _getConsensusReport(uint256 currentRefSlot, uint256 quorum) | ||||||||||||||||
| internal view returns (bytes32 report, int256 variantIndex, uint256 support) | ||||||||||||||||
| internal view returns (bytes32 report, uint256 support) | ||||||||||||||||
| { | ||||||||||||||||
| if (_reportingState.lastReportRefSlot != currentRefSlot) { | ||||||||||||||||
| // there were no reports for the current ref. slot | ||||||||||||||||
| return (ZERO_HASH, -1, 0); | ||||||||||||||||
| return (ZERO_HASH, 0); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| uint256 variantsLength = _reportVariantsLength; | ||||||||||||||||
| variantIndex = -1; | ||||||||||||||||
| report = ZERO_HASH; | ||||||||||||||||
| support = 0; | ||||||||||||||||
|
|
||||||||||||||||
| for (uint256 i = 0; i < variantsLength; ++i) { | ||||||||||||||||
| uint256 iSupport = _reportVariants[i].support; | ||||||||||||||||
| if (iSupport >= quorum) { | ||||||||||||||||
| variantIndex = int256(i); | ||||||||||||||||
| report = _reportVariants[i].hash; | ||||||||||||||||
| support = iSupport; | ||||||||||||||||
| break; | ||||||||||||||||
| address[] memory members = _memberAddresses; | ||||||||||||||||
|
|
||||||||||||||||
| for (uint256 i = 0; i < members.length; i++) { | ||||||||||||||||
|
Comment on lines
+1053
to
+1055
|
||||||||||||||||
| address[] memory members = _memberAddresses; | |
| for (uint256 i = 0; i < members.length; i++) { | |
| address[] storage members = _memberAddresses; | |
| uint256 length = members.length; | |
| for (uint256 i = 0; i < length; i++) { |
Copilot
AI
Mar 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_clearMemberReports copies _currentFrameReporters from storage into memory before iterating. Since this runs on the first report of each new frame, the extra memory copy is paid on-chain. Consider iterating _currentFrameReporters in storage (cache length first), then delete _currentFrameReporters after the loop, to reduce gas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new per-member reporting approach is intended to prevent unbounded growth from report-hash spamming, but there isn't a targeted regression test for this. Consider adding a test where one member submits many distinct hashes in the same frame and assert
getReportVariants()remains bounded and support counters remain correct.