Skip to content
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.

Sign up for GitHub

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

Check round ids #15183

Merged
merged 8 commits into from
Nov 12, 2024
+195 −18
Merged

Check round ids #15183

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d7c6f01
check rounds
eduard-cl Nov 11, 2024
b67bcdc
forge fmt
eduard-cl Nov 11, 2024
cc60767
modify transmit function
eduard-cl Nov 11, 2024
037dd75
Add tests
Oozyx Nov 11, 2024
3067d87
Merge pull request #15193 from smartcontractkit/feeds-project-additio…
eduard-cl Nov 12, 2024
ba122ad
transmit function improved
eduard-cl Nov 12, 2024
674785d
improve transmit logic + add 2 tests
eduard-cl Nov 12, 2024
c112e39
add comments + verify signatures early
eduard-cl Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 21 additions & 18 deletions contracts/src/v0.8/feeds/DualAggregator.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,8 +500,8 @@ contract DualAggregator is OCR2Abstract, OwnerIsCreator, AggregatorV2V3Interface
// max iterations the secondary proxy will be able to loop to sync with the primary rounds
uint32 internal s_maxSyncIterations;

// wether if the primary latest report has to be locked or not
bool internal s_primaryLocked;
// wether if the latest report was secondary or not
bool internal s_latestSecondary;

/**
* @notice indicates that a new report arrived from the secondary feed and the round id was updated
Expand Down Expand Up @@ -639,7 +639,7 @@ contract DualAggregator is OCR2Abstract, OwnerIsCreator, AggregatorV2V3Interface
// get the transmission
transmission = s_transmissions[latestAggregatorRoundId];
// in case the transmission was sent in this same block only by the secondary proxy, return the previous round id
if (s_primaryLocked && transmission.recordedTimestamp == block.timestamp) {
if (s_latestSecondary && transmission.recordedTimestamp == block.timestamp) {
return latestAggregatorRoundId - 1;
}
}
Expand Down Expand Up @@ -810,31 +810,36 @@ contract DualAggregator is OCR2Abstract, OwnerIsCreator, AggregatorV2V3Interface
(bool exist, uint32 roundId) = _doesReportExist(report_);
// In case the report exists, copy the round id and pay the transmitter
if (exist) {
// In case the round has already been processed by the secondary feed
if (s_hotVars.latestSecondaryRoundId >= roundId) {
revert StaleReport();
}

s_hotVars.latestSecondaryRoundId = roundId;
emit SecondaryRoundIdUpdated(roundId);

_payTransmitter(s_hotVars, report_.juelsPerFeeCoin, uint32(initialGas), msg.sender);
Oozyx marked this conversation as resolved.
Show resolved Hide resolved
return;
}
// In case the report doesn't exist, lock the primary feed
s_primaryLocked = true;
} else {
Copy link

@Oozyx Oozyx Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I think we could keep this else and use it for the case where the primary feed deals with receiving an existing, but latest report.

if (epochAndRound == s_hotVars.latestEpochAndRound && s_primaryLocked))

I see this if, else block as where the logic is evaluated for all edge cases where an existing report is sent to either feed. Early returns would happen here as well.

The rest of the function would be to handle and record new reports.

This is very subjective so feel free to disagree.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you develop more this idea? Not sure I'm following how it should be implemented.

// In case the sender is the primary proxy, unlock the latest report
s_primaryLocked = false;
}

// Report epoch and round
uint40 epochAndRound = uint40(uint256(reportContext[1]));

if (epochAndRound < s_hotVars.latestEpochAndRound) {
revert StaleReport();
}
if (epochAndRound != s_hotVars.latestEpochAndRound || !s_latestSecondary) {
if (epochAndRound <= s_hotVars.latestEpochAndRound) {
revert StaleReport();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm finding these conditions hard to follow. Can you include some comments explaining what different edge cases we are validating for?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments, it's true it's a bit hard to understand, because I inverted some conditionals to not have duplicated code. To help you understand the code, it's the same as this:

// In case the report is the same as the latest and it arrived throw the secondary feed,
// we have to validate it without transmitting a duplicated report
if (epochAndRound == s_hotVars.latestEpochAndRound && s_latestSecondary) {
  s_latestSecondary = isSecondary;
  _payTransmitter(s_hotVars, report_.juelsPerFeeCoin, uint32(initialGas), msg.sender);
  return;
}

// revert because it's older or already arrived throw the primary feed too (if not we would 
// have match the previous check)
if (epochAndRound <= s_hotVars.latestEpochAndRound) {
  revert StaleReport();
}

_report(s_hotVars, reportContext[0], epochAndRound, report_, isSecondary);

// Store if the latest report was secondary or not
s_latestSecondary = isSecondary;
_payTransmitter(s_hotVars, report_.juelsPerFeeCoin, uint32(initialGas), msg.sender);

The only difference is that I inverted the first conditional to not have this lines duplicated:

s_latestSecondary = isSecondary;
_payTransmitter(s_hotVars, report_.juelsPerFeeCoin, uint32(initialGas), msg.sender);


// Verify signatures attached to report
_verifySignatures(reportContext, report, rs, ss, rawVs);
// Verify signatures attached to report
_verifySignatures(reportContext, report, rs, ss, rawVs);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized we should always verify signatures. This should be placed under _validateReport

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? To optimize gas costs it's better if we only verify the signatures one time per report.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an attack vector. You can provide existing reports to the secondary feed that were not signed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but the existing reports have to have the same timestamp and answer than the one already validated. I thought the biggest benefit of this implementation was to not have to double check the signatures if a report has previously arrived by the other aggregator path.


int192 juelsPerFeeCoin = _report(s_hotVars, reportContext[0], epochAndRound, report_, isSecondary);
_payTransmitter(s_hotVars, juelsPerFeeCoin, uint32(initialGas), msg.sender);
_report(s_hotVars, reportContext[0], epochAndRound, report_, isSecondary);
}

// Store if the latest report was secondary or not
s_latestSecondary = isSecondary;
_payTransmitter(s_hotVars, report_.juelsPerFeeCoin, uint32(initialGas), msg.sender);
}

// helper function to validate the report data
Expand Down Expand Up @@ -953,7 +958,7 @@ contract DualAggregator is OCR2Abstract, OwnerIsCreator, AggregatorV2V3Interface
uint40 epochAndRound,
Report memory report,
bool isSecondary
) internal returns (int192 juelsPerFeeCoin) {
) internal {
if (report.observations.length > MAX_NUM_ORACLES) revert NumObservationsOutOfBounds();
// Offchain logic ensures that a quorum of oracles is operating on a matching set of at least
// 2f+1 observations. By assumption, up to f of those can be faulty, which includes being
Expand Down Expand Up @@ -1003,8 +1008,6 @@ contract DualAggregator is OCR2Abstract, OwnerIsCreator, AggregatorV2V3Interface
emit AnswerUpdated(median, hotVars.latestAggregatorRoundId, block.timestamp);

_validateAnswer(hotVars.latestAggregatorRoundId, median);

return report.juelsPerFeeCoin;
}

/**
Expand Down
Loading
Loading