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

Multi-whitelisted operators [audited] #299

Merged
merged 57 commits into from
Jul 7, 2024
Merged

Conversation

mtabasco
Copy link
Collaborator

The new feature allows operators to whitelist multiple EOAs/generic contracts and one external whitelisting contract.

EOAs/generic contracts

  • Managed by the internal SSV whitelisting module
  • An address can be whitelisted by many operators efficiently using bitmaps
  • Backward compatible with the current contracts data

Whitelisting contracts

  • One per operator
  • Must implement the interface ISSVWhitelistingContract and comply with ERC165

Comment on lines 28 to 29
if (!s.operators[operatorId].whitelisted) s.operators[operatorId].whitelisted = true;

Copy link
Collaborator

Choose a reason for hiding this comment

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

im not sure this is the best flow, what if you want to add a bunch of whitelists then make yourself public

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mtabasco is was decided by @arielssv we will remove this automatic whitelisting in all places

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andrew-blox Done. This change requires the operator owner to call the setOperatorsPrivateUnchecked function to make the operator private. If not, anyone can register validators using it even having whitelisted addresses.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@arielssv i know you already know just affirming it here

s.operatorsWhitelist[operatorId] = address(whitelistingContract);
}

emit OperatorWhitelistingContractUpdated(operatorIds, address(whitelistingContract));
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it worth to emit the old whitelist address as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it is enough to emit the new one. Using off-chain tools, all previous addresses can be retrieved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

agreed

Comment on lines 80 to 82
} else {
operator.whitelisted = true;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure i understand why this is in an else? seems like if there is not already an address there then we wont make whitelisted to true?

personally i dont think we should be changing whitelisted to true or false only if they request via the function that is made now (comment about this above)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mtabasco is was decided by @arielssv we will remove this automatic whitelisting in all places

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andrew-blox Done. This change requires the operator owner to call the setOperatorsPrivateUnchecked function to make the operator private.
Also, the logic of whitelisted changed a bit. Thanks

Copy link
Collaborator

@andrew-blox andrew-blox left a comment

Choose a reason for hiding this comment

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

There is no code nor tests dealing with whitelisted operators upon reactivation, this must be included

contracts/libraries/CoreLib.sol Show resolved Hide resolved
contracts/modules/SSVOperators.sol Outdated Show resolved Hide resolved
Comment on lines 77 to 80
function getWhitelistedOperators(
uint64[] calldata operatorIds,
address whitelistedAddress
) external view override returns (uint64[] memory whitelistedOperatorIds) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice if this function works with external contracts as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done: isAddressWhitelistedInWhitelistingContract

hardhat.config.ts Outdated Show resolved Hide resolved
tasks/deploy.ts Outdated Show resolved Hide resolved
tasks/deploy.ts Outdated Show resolved Hide resolved
@mtabasco mtabasco changed the title Multi-whitelisted operators Multi-whitelisted operators [freeze] Jun 9, 2024
@mtabasco mtabasco changed the title Multi-whitelisted operators [freeze] Multi-whitelisted operators [auditing] Jun 9, 2024
@mtabasco mtabasco changed the title Multi-whitelisted operators [auditing] Multi-whitelisted operators [audited] Jul 3, 2024
@andrew-blox andrew-blox self-requested a review July 3, 2024 08:47
Copy link
Contributor

@Mohsen-T Mohsen-T left a comment

Choose a reason for hiding this comment

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

I carefully reviewed every modified or added code, and everything looks good to merge into the branch

@liorrutenberg liorrutenberg self-requested a review July 7, 2024 11:20
@liorrutenberg liorrutenberg merged commit 085874c into main Jul 7, 2024
5 checks passed
@liorrutenberg liorrutenberg deleted the multi-op-whitelist branch July 7, 2024 11:22
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.

4 participants