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.

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

Address ChainSecurity questions #90

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

danielattilasimon
Copy link
Contributor

Eliminate unnecessary corner cases & special checks that should never be encountered/needed in normal operation.
Fix code comment about states in which user can vote positively.

Addresses ChainSecurity audit 7.1, 7.5 & 7.6.
Closes IR-09 & IR-10.

They should never be encountered in normal operation.

Addresses ChainSecurity audit 7.1 & 7.5.
Addresses ChainSecurity audit 7.6.
}

// should not revert under any input
function test_averageAge(uint120 _currentTimestamp, uint120 _timestamp) public {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing useless test.

}

// should not revert under any input
function test_calculateAverageTimestamp(
Copy link
Contributor Author

@danielattilasimon danielattilasimon Nov 29, 2024

Choose a reason for hiding this comment

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

Removing harmful test. It should very much revert when passed nonsensical arguments.

The average timestamp is a weighted average of the form:

t_avg = SUM_i(t_i * lqty_i) / SUM_i(lqty_i)

It is always legal to add any number of new (t_j, lqty_j) pairs to an existing weighted average, in which case the new average will be:

t_avg' = (t_avg * SUM_i(lqty_i) + SUM(t_j * lqty_j)) / (SUM_i(lqty_i) + SUM_j(lqty_j))
       = (t_avg * SUM_i(lqty_i) + [SUM(t_j * lqty_j) / SUM_j(lqty_j)] * SUM_j(lqty_j)) / (SUM_i(lqty_i) + SUM_j(lqty_j))

Note: the parameters of the function _calculateAverageTimestamp() correspond to the following terms:

  • _prevOuterAverageTimestamp => t_avg
  • _newInnerAverageTimestamp => SUM(t_j * lqty_j) / SUM_j(lqty_j)
  • _prevLQTYBalance => SUM_i(lqty_i)
  • _newLQTYBalance => SUM_i(lqty_i) + SUM_j(lqty_j)

This explains the seemingly overcomplicated second form of t_avg' above.

However, when removing previously allocated amounts, even if we allow partial deallocation, the parameter _newInnerAverageTimestamp itself must be a valid weighted average of the form:

_newInnerAverageTimestamp = SUM_i(t_i * lqtyDecrease_i) / SUM_i(lqtyDecrease_i)

where forall i. lqtyDecrease_i <= lqty_i. Failing to adhere to this constraint can and should result in reverts.

Copy link
Contributor

@bingen bingen left a comment

Choose a reason for hiding this comment

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

Looks good!

return uint120(
_newInnerAverageTimestamp + _prevOuterAverageTimestamp * _prevLQTYBalance / _newLQTYBalance
- _newInnerAverageTimestamp * _prevLQTYBalance / _newLQTYBalance
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!! ❤️

@danielattilasimon danielattilasimon merged commit 48afaad into main Dec 3, 2024
3 checks passed
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.

2 participants