Revert "feat: custom gas token" #10384
Conversation
This reverts commit a6074a7.
WalkthroughWalkthroughThe recent update primarily involves renaming Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files ignored due to path filters (23)
Files selected for processing (41)
Files skipped from review due to trivial changes (12)
Additional Context UsedLanguageTool (2)
Learnings (13)
Additional comments not posted (64)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Out of diff range and nitpick comments (25)
packages/contracts-bedrock/invariant-docs/SystemConfig.md (1)
Line range hint
6-6: Remove the extra blank line to maintain consistency in document formatting.-packages/contracts-bedrock/scripts/DeployConfig.s.sol (9)
Line range hint
1-1: Ensure SPDX license identifier is correct and matches project standards.
Line range hint
3-3: Consider updating Solidity version to the latest stable release to benefit from improved language features and security fixes.
Line range hint
7-7: Ensure the contract description is accurate and reflects the current functionality after reverting changes related to the custom gas token.
Line range hint
9-9: Consider adding more detailed comments to explain the purpose and usage of each public and internal variable.
Line range hint
11-11: Review the necessity of all public variables. Consider reducing visibility if they do not need to be accessed externally.
Line range hint
13-13: Ensure that error messages are clear and provide sufficient information to diagnose issues.
Line range hint
15-15: Consider implementing more robust error handling for file reading operations to prevent potential disruptions in deployment.
[!TIP]
Codebase VerificationThe verification process has identified the use of
vm.ffi()in theDeployConfig.s.solfile, which is a method for executing external commands in Solidity scripts. This confirms the presence of external command usage, which was the focus of the review comment regarding potential security vulnerabilities.Given this finding, it is crucial to ensure that the inputs to
vm.ffi()are properly validated to avoid security risks such as command injection. The review comment is accurate in highlighting this concern.Conclusion:
- The use of
vm.ffi()inDeployConfig.s.solconfirms the need for robust input validation to mitigate potential security vulnerabilities associated with executing external commands.Analysis chain
Line range hint
19-19: Check for potential security vulnerabilities in the use of external commands and ensure that input validation is robust.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential security vulnerabilities in external command usage in DeployConfig.s.sol ast-grep --lang solidity --pattern $'vm.ffi($$$)' packages/contracts-bedrock/scripts/DeployConfig.s.solLength of output: 221
Script:
#!/bin/bash # Description: Check for potential security vulnerabilities in external command usage in DeployConfig.s.sol using rg rg 'vm\.ffi\(' packages/contracts-bedrock/scripts/DeployConfig.s.solLength of output: 154
Line range hint
21-21: Ensure that all fallback values are sensible and align with the system's operational requirements.packages/contracts-bedrock/scripts/Artifacts.s.sol (9)
Line range hint
1-1: Ensure SPDX license identifier is correct and matches project standards.
Line range hint
3-3: Consider updating Solidity version to the latest stable release to benefit from improved language features and security fixes.
Line range hint
7-7: Ensure the contract description is accurate and reflects the current functionality after reverting changes related to the custom gas token.
Line range hint
9-9: Consider adding more detailed comments to explain the purpose and usage of each public and internal variable.
Line range hint
11-11: Review the necessity of all public variables. Consider reducing visibility if they do not need to be accessed externally.
Line range hint
13-13: Ensure that error messages are clear and provide sufficient information to diagnose issues.
Line range hint
15-15: Consider implementing more robust error handling for file reading operations to prevent potential disruptions in deployment.
[!TIP]
Codebase VerificationThe verification process has identified the use of
vm.ffiin theArtifacts.s.solfile, which is a method for executing external commands in the Foundry testing framework. This confirms the presence of external command execution as highlighted in the review comment. Given this, it is crucial to ensure that the inputs to these commands are properly validated to avoid security vulnerabilities.Action Required:
- Review the usage of
vm.ffiin theArtifacts.s.solfile to ensure that all inputs are properly validated before being passed to external commands.Analysis chain
Line range hint
19-19: Check for potential security vulnerabilities in the use of external commands and ensure that input validation is robust.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential security vulnerabilities in external command usage in Artifacts.s.sol ast-grep --lang solidity --pattern $'vm.ffi($$$)' packages/contracts-bedrock/scripts/Artifacts.s.solLength of output: 218
Script:
#!/bin/bash # Description: Check for potential security vulnerabilities in external command usage in Artifacts.s.sol using ripgrep rg 'vm\.ffi' packages/contracts-bedrock/scripts/Artifacts.s.solLength of output: 160
Line range hint
21-21: Ensure that all fallback values are sensible and align with the system's operational requirements.packages/contracts-bedrock/src/universal/CrossDomainMessenger.sol (1)
Line range hint
1-1: Ensure SPDX license identifier is correct and matches project standards.packages/contracts-bedrock/src/universal/StandardBridge.sol (1)
Line range hint
1-1: Ensure SPDX license identifier is correct and matches project standards.packages/contracts-bedrock/test/L2/L2StandardBridge.t.sol (1)
140-140: Ensure that the error message inwithdrawfunction is accurate and provides clear guidance on the issue.Consider refining the error message to include more details about the expected and actual values.
packages/contracts-bedrock/test/L1/OptimismPortal.t.sol (1)
Line range hint
519-519: Incorrect usage ofblockhashfunction.- blockhash(block.number) + blockhash(block.number - 1)The
blockhashfunction does not return valid hashes for the current block number (block.number) or any block number in the future (block.number + N). It only works for the 256 most recent blocks, excluding the current one. Therefore, you should useblock.number - 1to get the hash of the most recent block.packages/contracts-bedrock/scripts/Deploy.s.sol (2)
Line range hint
1-1: Consider parameterizing the hardcoded addresses forSafeProxyFactoryandSafeto enhance flexibility and maintainability.- address safeProxyFactory = 0xa6B71E26C5e0845f74c812102Ca7114b6a896AB2; - address safeSingleton = 0xd9Db270c1B5E3Bd161E8c8503c55cEABeE709552; + address safeProxyFactory = _safeProxyFactoryAddress; // Add this variable at the contract level + address safeSingleton = _safeSingletonAddress; // Add this variable at the contract level
Line range hint
1-1: Direct use ofmsg.senderindeployAddressManagercould lead to security risks. Consider using a more secure method to handle ownership.- require(manager.owner() == msg.sender); + address expectedOwner = _expectedOwner; // Define this variable securely + require(manager.owner() == expectedOwner, "Unauthorized: Caller is not the expected owner");
Reverts #10143
This was merged too soon—we will merge once the fault proofs release is cut, since they both touch the SystemConfig