Skip to content

Conversation

@nicholaspai
Copy link
Member

@nicholaspai nicholaspai commented Mar 15, 2022

Issue:

  • [L08] Time is cast unsafely
    • In the HubPool function _updateAccumulatedLpFees, the return value of getCurrentTime() is cast to
      a uint32 value. This means that the value will be truncated to fit within 32 bits, and at some point
      around Feb 6, 2106, it will "roll over" and the value returned by casting to uint32 will drop down to 0.
      This will set pooledToken.lastLpFeeUpdate to a much lower number than the previous
      lastLpFeeUpdate. Any subsequent time _getAccumulatedFees is called, the
      timeFromLastInteraction calculation will be exceedingly high, and all "undistributed" fees will be
      accounted for as accumulated.
    • Again, note that this issue will only occur starting in the year 2106. Consider changing the size of the
      cast from uint32 to a larger number, like uint64. This should be more than enough to not encounter
      limits within a reasonably distant future. Alternatively, consider documenting the behavior and defining
      a procedure for what to do if the system is still in operation when the uint32 limit is hit, or for shutting
      down the system before the year 2106.

Solution:

  • We use uint32 time-related variables to save gas costs by packing variables smartly, and we do intend to deprecate this contract by 2106. So we add a disclaimer at the top of the contract that it should be deprecated by 2106 and steps for doing so.

@nicholaspai nicholaspai added the OZ Audit - March Resolves issue discovered in March 2022 OZ Audit label Mar 15, 2022
@nicholaspai nicholaspai changed the title improve: Change time-related variables to type uint64 improve: [L08] Change time-related variables to type uint64 Mar 15, 2022
@nicholaspai nicholaspai changed the title improve: [L08] Change time-related variables to type uint64 improve: [L08] Add comment about shutting down contract before uint32 timestamps roll over in year 2106 Mar 15, 2022
Copy link
Contributor

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OZ Audit - March Resolves issue discovered in March 2022 OZ Audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants