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

fix(IdRegistry): added contract validation to setStorageRegistry #454

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0xObsidian
Copy link

@0xObsidian 0xObsidian commented Dec 27, 2024

Motivation

This PR closes #440
setStorageRegistry was missing validation checks for the new address, which could lead to setting invalid contracts or non-contract addresses as the storage registry.

Change Summary

  • Added zero address check using revert
  • Added contract existence check
  • Used revert with custom errors instead of require() for:
    • Gas efficiency (4-byte selector vs string storage)
    • Better ABI compatibility via custom errors
  • Updated registry tests to align with the current implementation
  • Fixed assumeNotPrecompile issue
  • Ran forge formatting on all modified files in this PR scope

Testing the fix

Fetch this PR branch and from the root:

Add foundry-rs/forge-std

forge install foundry-rs/forge-std

Run test

forge test --match-test testFuzzSetStorageRegistry -vvv

Merge Checklist

Choose all relevant options below by adding an x now or at any time before submitting for review


PR-Codex overview

This PR focuses on improving function signatures by making them more consistent and readable, particularly by changing certain functions to view. It also includes minor refactoring for clarity and better code organization across various test suites and contracts.

Detailed summary

  • Changed several function signatures to view in KeyRegistryTestSuite.sol, IdGatewayTestSuite.sol, and IdRegistryTestSuite.sol.
  • Refactored function parameters for clarity in IdGateway.sol and IdRegistryTestSuite.sol.
  • Added assumptions in testFuzzSetStorageRegistry to ensure valid inputs.
  • Updated the _signDigest function to be pure.
  • Improved readability by breaking long parameter lists into multiple lines in various functions.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

The setStorageRegistry function was missing validation checks
for the new address, which could lead to setting invalid contracts
or non-contract addresses as the storage registry.

Description
----------
- Added zero address check using revert
- Added contract existence check
- Used revert with custom errors instead of require() for:
  - Gas efficiency (4-byte selector vs string storage)
  - Better ABI compatibility via custom errors
- Updated registry tests to align with the current implementation
- Fixed `assumeNotPrecompile` issue
- Ran forge formatting on all modified files in this PR scope

Testing the fix
----------------
Fetch this PR branch and from the root:

Add `foundry-rs/forge-std`
```
forge install foundry-rs/forge-std
```

Run test
```
forge test --match-test testFuzzSetStorageRegistry -vvv

```
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.

bug; Unchecked External Call in setStorageRegistry in src/IdGateway.sol
1 participant