-
Notifications
You must be signed in to change notification settings - Fork 7
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 multi-contrib nodes setting contract as operator of node #92
Fix multi-contrib nodes setting contract as operator of node #92
Conversation
sn.operator = contributors[0].staker.addr; | ||
for (uint256 i = 0; i < contributors.length; i++) | ||
sn.operator = operator; | ||
for (uint256 i = 0; i < contributors.length; ) { |
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.
Why did the i++
get dropped? (I don't see how this isn't an infinite loop without 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.
Oh, I see, I think this was a merge conflict with the ongoing audit response branch (which drops the i++ but has an unchecked { i += 1; }
in the body).
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 pushed a fix.
contracts/ServiceNodeRewards.sol
Outdated
sn.contributors.push(contributors[i]); | ||
unchecked { i += 1 }; |
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.
unchecked { i++; }
c7a7c11
to
7a24a9e
Compare
Thanks, have squashed the bugs back to the base commit (and fixed the syntax error in the |
In my integration tests curiously multi-contributor contracts were failing until I realised that we were using
msg.sender
as the operator which is incorrect in the multi-contributor flow.When you have a multi-contributor contract, that calls
addBLSPublicKey
on the rewards contract on the operator's behalf. This means thatmsg.sender
is set to the multi-contribution contract. Then, inaddBLSPublicKey
we'd use themsg.sender
tovalidateProofOfPossession
but also emit aNewServiceNodeV2
log with the contract address.This is incorrect and would prevent the operator from being able to exit the node. It'd also fail checks in core where it expects the 1st contributor to be the operator. The fix here is to correctly use
msg.sender
for transferring the stake and the 1st contributor as the operator for the SN metadata.