Skip to content

Conversation

BenSparksCode
Copy link
Contributor

We track the ETH value of gas used by solvers for analytics purposes. There was a bug in this tracking code where the value passed into _updateAnalytics() was already in the form of the ETH cost (i.e. gasUsed * tx.gasprice) but was multiplied by tx.gasprice again in the helper. This caused an overflow.

Fixes:

  • Remove 2nd * tx.gasprice operation.
  • Name args more clearly.
  • Use OZ SafeCast for clearer errors around overflow due to uint64 datatype.
  • Fix tests.

Copy link
Contributor

@jj1980a jj1980a left a comment

Choose a reason for hiding this comment

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

lgtm

@BenSparksCode
Copy link
Contributor Author

BenSparksCode commented Aug 21, 2024

Oof this pushes Atlas over the contract size limit. 77 bytes over. I suspect due to SafeCast.

Will make a separate branch to bump OZ version from 4.8 to 5.0, which moves to custom errors. That should free up some space.

Base automatically changed from forge-fmt to main August 21, 2024 12:11
@BenSparksCode
Copy link
Contributor Author

Atlas is back under size limit now (#413 lowered sizes a bit), so this is ready for final review.

@BenSparksCode BenSparksCode merged commit 79ff6a4 into main Aug 22, 2024
@BenSparksCode BenSparksCode deleted the fix-analytics-overflow branch August 22, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants