-
Notifications
You must be signed in to change notification settings - Fork 145
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
fix: addToDelegationPool token transfer (OZ C-01 and C-02) #988
Conversation
fix: addToDelegationPool token transfer (OZ C-01 and C-02)
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
@@ -278,6 +278,8 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain { | |||
uint256 tokens | |||
) external override notPaused { | |||
require(tokens != 0, HorizonStakingInvalidZeroTokens()); | |||
require(msg.sender == verifier || msg.sender == address(_graphPayments()), HorizonStakingInvalidDelegationPoolSender(msg.sender)); |
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.
I know OZ suggested this, but I'd argue we don't need it:
- We would be limiting potential use cases we don't know about
- Documentation already mentions that delegators SHOULD NOT use this function and it's not like we will be creating client side apps to facilitate calling it.
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.
I think this makes sense but worth discussing with OZ just in case, it could increase the risk of rounding attacks etc.
(If we do go ahead as it is, let's rename the PR without the "access control" part)
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.
I've renamed and will let OZ know so they can give their feedback.
54b150c
to
f4f9bad
Compare
No description provided.