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

Allowlists checks #2770

Closed
Tracked by #2530
grarco opened this issue Feb 28, 2024 · 1 comment · Fixed by #2819
Closed
Tracked by #2530

Allowlists checks #2770

grarco opened this issue Feb 28, 2024 · 1 comment · Fixed by #2819
Assignees
Labels
gas pre-mainnet Must happen before mainnet. security wasm

Comments

@grarco
Copy link
Contributor

grarco commented Feb 28, 2024

The followings are to be addressed:

  • Currently we don't verify the allowlist before running vps (only when updating one in storage), but this could be a problem in case one of the vps was removed from the allowlist by a governance proposal (this is not guaranteed, it depends on which keys are removed and which are not). For extra safety I think we should validate every vp before running it anyway
  • The allowlist check for transactions is done in finalize_block. Once Bat/feat/remove tx queue #2627 is merged the execution of both the wrapper and the inner will happen in the same block meaning that this check will also prevent the execution of the wrapper and therefore fee payment. Moreover, this early check prevents replay protection from running some logic and decides whether to add the hash of the tx or not based on the section commitments. We should definitely address the former issue and try to solve the latter if possible too. In addition, after Bat/feat/remove tx queue #2627, we could also replicate the allowlist check in process_proposal and mempool too
@grarco grarco added gas security pre-mainnet Must happen before mainnet. labels Feb 28, 2024
@grarco grarco added the wasm label Feb 28, 2024
@grarco grarco self-assigned this Mar 4, 2024
@grarco
Copy link
Contributor Author

grarco commented Mar 5, 2024

We decided to implement a soft version of vps removal from the allowlist: we'll remove the hash so that new accounts cannot be created with this vp but we'll keep the code so that the old accounts can still run it. This means that point 1 should not be implemented cause this would collide with the desired behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gas pre-mainnet Must happen before mainnet. security wasm
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants