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

Check if namespaces exist in access control checks #2105

Closed
yonadaa opened this issue Jan 10, 2024 · 3 comments
Closed

Check if namespaces exist in access control checks #2105

yonadaa opened this issue Jan 10, 2024 · 3 comments

Comments

@yonadaa
Copy link
Contributor

yonadaa commented Jan 10, 2024

It was recently suggested that the requireAccess function should explicitly check if the namespace exists using:

ResourceIds.getExists(toNamespaceId))

This would ensure any access control checks on non-existent namespaces revert, which is expected because no one has access to them yet. This prevents any "premature" interactions like sending ETH to an unregistered namespace.

However this is a hot code path so this check would add some gas overhead.

@alvrs
Copy link
Member

alvrs commented Jan 10, 2024

i'd prefer to keep those checks unbundled so we can more granularly use them (ie only check existence of a namespace once, but then require multiple actors to have access to it)

@holic
Copy link
Member

holic commented Jan 11, 2024

i'd prefer to keep those checks unbundled so we can more granularly use them (ie only check existence of a namespace once, but then require multiple actors to have access to it)

Do we have places where we do multiple access checks? What's the cost of a "warm" look up of the namespace existing?

@alvrs
Copy link
Member

alvrs commented Jan 11, 2024

Do we have places where we do multiple access checks?

I'm not sure if we do in the current codebase but i can imagine this being used in modules.

One of big themes of the recent audit was bundling too much functionality in one thing rather than having one thing do one thing only, I feel like this goes in a similar direction. I'd rather do those checks explicitly in consuming code to make it clear what's going on than bundling both checks in one checkForExistenceAndAccess function.

@holic holic closed this as completed Jan 12, 2024
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

No branches or pull requests

3 participants