Platform: Code4rena
Start Date: 10/05/2024
Pot Size: $300,500 USDC
Total HM: 4
Participants: 27
Period: 17 days
Judge: Picodes
Total Solo HM: 1
Id: 375
League: ETH
Rank: 16/27
Findings: 1
Award: $0.00
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: Sathish9098
Also found by: Audinarey, Dup1337, K42, KupiaSec, LessDupes, Rhaydden, SpicyMeatball, Takarez, ZanyBonzy, bronze_pickaxe, carlitox477, dontonka, forgebyola, fyamf, guhu95, hihen, josephdara, ladboy233, slvDev, twcctop, xuwinnie, zanderbyte
0 USDC - $0.00
Total 91 instances over 13 issues:
| ID | Issue | Instances |
|---|---|---|
| [L-01] | Passing concat() with dynamic arguments to a hash can cause collisions | 2 |
| [L-02] | Variables shadowing other definitions | 2 |
| [L-03] | Upgradable contracts need a constructor to init and lock the implementation contract | 3 |
| [L-04] | Missing zero address check in initializer | 1 |
| [L-05] | State variables not limited to reasonable values | 2 |
| [L-06] | Contracts are not using their OZ Upgradeable counterparts | 2 |
| [L-07] | Vulnerable versions of packages are being used | 8 |
| [L-08] | Constructor / initialization function lacks parameter validation | 17 |
| [L-09] | Unsafe solidity low-level call can cause gas grief attack | 1 |
| [L-10] | Functions calling contracts/addresses with transfer hooks should be protected by reentrancy guard | 9 |
| [L-11] | Critical functions should be controlled by time locks | 11 |
| [L-12] | Duplicate data may be pushed to the array | 1 |
| [L-13] | Code does not follow the best practice of check-effects-interaction | 32 |
Total 1078 instances over 68 issues:
| ID | Issue | Instances |
|---|---|---|
| [N-01] | Import declarations should import specific identifiers, rather than the whole file | 153 |
| [N-02] | Visibility of state variables is not explicitly defined | 1 |
| [N-03] | Names of private/internal functions should be prefixed with an underscore | 99 |
| [N-04] | Names of private/internal state variables should be prefixed with an underscore | 10 |
| [N-05] | Use of override is unnecessary | 33 |
| [N-06] | Add inline comments for unnamed parameters | 5 |
| [N-07] | Consider providing a ranged getter for array state variables | 1 |
| [N-08] | Consider splitting complex checks into multiple steps | 1 |
| [N-09] | Cast to bytes for clearer semantic meaning | 1 |
| [N-10] | Missing checks for empty bytes when updating bytes state variables | 4 |
| [N-11] | Consider adding a block/deny-list | 4 |
| [N-12] | Constants/Immutables redefined elsewhere | 4 |
| [N-13] | Convert simple if-statements to ternary expressions | 2 |
| [N-14] | Contracts should each be defined in separate files | 2 |
| [N-15] | Contract name does not match its filename | 3 |
| [N-16] | UPPER_CASE names should be reserved for constant/immutable variables | 5 |
| [N-17] | Consider emitting an event at the end of the constructor | 10 |
| [N-18] | Empty function body without comments | 3 |
| [N-19] | Events are emitted without the sender information | 34 |
| [N-20] | Inconsistent floating version pragma | 4 |
| [N-21] | Function state mutability can be restricted to pure | 2 |
| [N-22] | Imports could be organized more systematically | 28 |
| [N-23] | There is no need to initialize bool variables with false | 1 |
| [N-24] | @openzeppelin/contracts should be upgraded to a newer version | 20 |
| [N-25] | Lib @openzeppelin/contracts-upgradeable should be upgraded to a newer version | 3 |
| [N-26] | Expressions for constant values should use immutable rather than constant | 1 |
| [N-27] | Consider moving duplicated strings to constants | 22 |
| [N-28] | Contracts containing only utility functions should be made into libraries | 3 |
| [N-29] | Functions should be named in mixedCase style | 7 |
| [N-30] | Variable names for immutables should use UPPER_CASE_WITH_UNDERSCORES | 14 |
| [N-31] | Names of public/external functions and state variables should NOT be prefixed with underscore | 1 |
| [N-32] | Overridden function has no body | 2 |
| [N-33] | Named imports of parent contracts are missing | 25 |
| [N-34] | Constants should be put on the left side of comparisons | 106 |
| [N-35] | else-block not required | 12 |
| [N-36] | Large multiples of ten should use scientific notation | 1 |
| [N-37] | TODOs left in the code | 1 |
| [N-38] | High cyclomatic complexity | 1 |
| [N-39] | Typos | 18 |
| [N-40] | Consider bounding input array length | 4 |
| [N-41] | Unnecessary casting | 11 |
| [N-42] | Unused contract variables | 1 |
| [N-43] | Unused function parameters | 2 |
| [N-44] | Unused named return | 2 |
| [N-45] | Use delete instead of assigning values to false | 1 |
| [N-46] | Consider using delete rather than assigning zero to clear values | 3 |
| [N-47] | Use the latest Solidity version | 24 |
| [N-48] | Use transfer libraries instead of low level calls to transfer native tokens | 1 |
| [N-49] | Using underscore at the end of variable name | 27 |
| [N-50] | Use a struct to encapsulate multiple function parameters | 9 |
| [N-51] | Returning a struct instead of a bunch of variables is better | 7 |
| [N-52] | Contract variables should have comments | 11 |
| [N-53] | Missing event when a state variables is set in constructor | 3 |
| [N-54] | Empty bytes check is missing | 5 |
| [N-55] | Don't define functions with the same name in a contract | 7 |
| [N-56] | Multiple mappings with same keys can be combined into a single struct mapping for readability | 7 |
| [N-57] | Do not cache immutable variables | 5 |
| [N-58] | Missing event for critical changes | 17 |
| [N-59] | Consider adding emergency-stop functionality | 6 |
| [N-60] | Avoid the use of sensitive terms | 63 |
| [N-61] | Missing checks for uint state variable assignments | 6 |
| [N-62] | Use the Modern Upgradeable Contract Paradigm | 11 |
| [N-63] | Large or complicated code bases should implement invariant tests | 1 |
| [N-64] | The default value is manually set when it is declared | 23 |
| [N-65] | Contracts should have all public/external functions exposed by interfaces | 9 |
| [N-66] | Top-level declarations should be separated by at least two lines | 120 |
| [N-67] | Consider adding formal verification proofs | 39 |
| [N-68] | Use scopes sparingly | 6 |
concat() with dynamic arguments to a hash can cause collisionsPassing bytes.concat()/string.concat() with multiple dynamic arguments can cause hash collisions. It is recommended to use abi.encode() instead which will pad items to 32 bytes and prevent hash collisions.
There are 2 instances:
717: return (keccak256(bytes.concat(header, data)), timeBounds); 744: keccak256(bytes.concat(header, DATA_BLOB_HEADER_FLAG, abi.encodePacked(dataHashes))),
A variable declaration shadowing any other existing definition is error-prone. It can cause confusion for developers, potentially introduce bugs, and reduce the readability and maintainability.
There are 2 instances:
/// @audit Shadows `function stakerCount()` 344: uint64 stakerCount = ROLLUP_READER.stakerCount(); /// @audit Shadows `state variable rollup` 516: RollupProxy rollup = new RollupProxy{salt: rollupSalt}();
An uninitialized contract can be taken over by an attacker. For an upgradable contract, this applies to both the proxy and its implementation contract, which may impact the proxy.
To prevent the implementation contract from being used, we should trigger the initialization in the constructor to automatically lock it when it is deployed.
For contracts that inherit Initializable, the _disableInitializers() function is suggested to do this job.
There are 3 instances:
/// @audit Missing constructor to initialize the implementation contract 15: contract RollupAdminLogic is RollupCore, IRollupAdmin, DoubleLogicUUPSUpgradeable {
/// @audit Missing constructor to initialize the implementation contract 22: abstract contract RollupCore is IRollupCore, PausableUpgradeable {
/// @audit Missing constructor to initialize the implementation contract 15: contract RollupUserLogic is RollupCore, UUPSNotUpgradeable, IRollupUser {
Consider adding a zero address check for each address type parameter in initializer.
There is 1 instance:
/// @audit `_stakeToken not checked` 305: function initialize( 306: IAssertionChain _assertionChain, 307: uint64 _challengePeriodBlocks, 308: IOneStepProofEntry _oneStepProofEntry, 309: uint256 layerZeroBlockEdgeHeight, 310: uint256 layerZeroBigStepEdgeHeight, 311: uint256 layerZeroSmallStepEdgeHeight, 312: IERC20 _stakeToken, 313: address _excessStakeReceiver, 314: uint8 _numBigStepLevel, 315: uint256[] calldata _stakeAmounts 316: ) public initializer {
Consider adding appropriate minimum/maximum value checks to ensure that the following state variables can never be used to excessively harm users, including via griefing.
There are 2 instances:
328: challengePeriodBlocks = _challengePeriodBlocks;
205: minimumAssertionPeriod = newPeriod;
The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behavior.
Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts. See this for list of available upgradeable contracts
There are 2 instances:
15: import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
8: import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
This project is using specific package versions which are vulnerable to the specific CVEs listed below. Consider switching to more recent versions of these packages that don't have these vulnerabilities.
CVE-2023-40014 - MEDIUM - (@openzeppelin/contracts >=4.0.0 <4.9.3): OpenZeppelin Contracts is a library for secure smart contract development. Starting in version 4.0.0 and prior to version 4.9.3, contracts using ERC2771Context along with a custom trusted forwarder may see _msgSender return address(0) in calls that originate from the forwarder with calldata shorter than 20 bytes. This combination of circumstances does not appear to be common, in particular it is not the case for MinimalForwarder from OpenZeppelin Contracts, or any deployed forwarder the team is aware of, given that the signer address is appended to all calls that originate from these forwarders. The problem has been patched in v4.9.3.
CVE-2023-40014 - MEDIUM - (@openzeppelin/contracts-upgradeable >=4.0.0 <4.9.3): OpenZeppelin Contracts is a library for secure smart contract development. Starting in version 4.0.0 and prior to version 4.9.3, contracts using ERC2771Context along with a custom trusted forwarder may see _msgSender return address(0) in calls that originate from the forwarder with calldata shorter than 20 bytes. This combination of circumstances does not appear to be common, in particular it is not the case for MinimalForwarder from OpenZeppelin Contracts, or any deployed forwarder the team is aware of, given that the signer address is appended to all calls that originate from these forwarders. The problem has been patched in v4.9.3.
CVE-2023-34459 - MEDIUM - (@openzeppelin/contracts >=4.7.0 <4.9.2): OpenZeppelin Contracts is a library for smart contract development. Starting in version 4.7.0 and prior to version 4.9.2, when the verifyMultiProof, verifyMultiProofCalldata, procesprocessMultiProof, or processMultiProofCalldat functions are in use, it is possible to construct merkle trees that allow forging a valid multiproof for an arbitrary set of leaves. A contract may be vulnerable if it uses multiproofs for verification and the merkle tree that is processed includes a node with value 0 at depth 1 (just under the root). This could happen inadvertedly for balanced trees with 3 leaves or less, if the leaves are not hashed. This could happen deliberately if a malicious tree builder includes such a node in the tree. A contract is not vulnerable if it uses single-leaf proving (verify, verifyCalldata, processProof, or processProofCalldata), or if it uses multiproofs with a known tree that has hashed leaves. Standard merkle trees produced or validated with the @openzeppelin/merkle-tree library are safe. The problem has been patched in version 4.9.2. Some workarounds are available. For those using multiproofs: When constructing merkle trees hash the leaves and do not insert empty nodes in your trees. Using the @openzeppelin/merkle-tree package eliminates this issue. Do not accept user-provided merkle roots without reconstructing at least the first level of the tree. Verify the merkle tree structure by reconstructing it from the leaves.
CVE-2023-34459 - MEDIUM - (@openzeppelin/contracts-upgradeable >=4.7.0 <4.9.2): OpenZeppelin Contracts is a library for smart contract development. Starting in version 4.7.0 and prior to version 4.9.2, when the verifyMultiProof, verifyMultiProofCalldata, procesprocessMultiProof, or processMultiProofCalldat functions are in use, it is possible to construct merkle trees that allow forging a valid multiproof for an arbitrary set of leaves. A contract may be vulnerable if it uses multiproofs for verification and the merkle tree that is processed includes a node with value 0 at depth 1 (just under the root). This could happen inadvertedly for balanced trees with 3 leaves or less, if the leaves are not hashed. This could happen deliberately if a malicious tree builder includes such a node in the tree. A contract is not vulnerable if it uses single-leaf proving (verify, verifyCalldata, processProof, or processProofCalldata), or if it uses multiproofs with a known tree that has hashed leaves. Standard merkle trees produced or validated with the @openzeppelin/merkle-tree library are safe. The problem has been patched in version 4.9.2. Some workarounds are available. For those using multiproofs: When constructing merkle trees hash the leaves and do not insert empty nodes in your trees. Using the @openzeppelin/merkle-tree package eliminates this issue. Do not accept user-provided merkle roots without reconstructing at least the first level of the tree. Verify the merkle tree structure by reconstructing it from the leaves.
CVE-2023-30542 - HIGH - (@openzeppelin/contracts >=4.7.0 <4.8.2): OpenZeppelin Contracts is a library for secure smart contract development. The proposal creation entrypoint (propose) in GovernorCompatibilityBravo allows the creation of proposals with a signatures array shorter than the calldatas array. This causes the additional elements of the latter to be ignored, and if the proposal succeeds the corresponding actions would eventually execute without any calldata. The ProposalCreated event correctly represents what will eventually execute, but the proposal parameters as queried through getActions appear to respect the original intended calldata. This issue has been patched in 4.8.3. As a workaround, ensure that all proposals that pass through governance have equal length signatures and calldatas parameters.
CVE-2023-30542 - HIGH - (@openzeppelin/contracts-upgradeable >=4.7.0 <4.8.2): OpenZeppelin Contracts is a library for secure smart contract development. The proposal creation entrypoint (propose) in GovernorCompatibilityBravo allows the creation of proposals with a signatures array shorter than the calldatas array. This causes the additional elements of the latter to be ignored, and if the proposal succeeds the corresponding actions would eventually execute without any calldata. The ProposalCreated event correctly represents what will eventually execute, but the proposal parameters as queried through getActions appear to respect the original intended calldata. This issue has been patched in 4.8.3. As a workaround, ensure that all proposals that pass through governance have equal length signatures and calldatas parameters.
CVE-2023-30541 - MEDIUM - (@openzeppelin/contracts >=3.2.0 <4.8.3): OpenZeppelin Contracts is a library for secure smart contract development. A function in the implementation contract may be inaccessible if its selector clashes with one of the proxy's own selectors. Specifically, if the clashing function has a different signature with incompatible ABI encoding, the proxy could revert while attempting to decode the arguments from calldata. The probability of an accidental clash is negligible, but one could be caused deliberately and could cause a reduction in availability. The issue has been fixed in version 4.8.3. As a workaround if a function appears to be inaccessible for this reason, it may be possible to craft the calldata such that ABI decoding does not fail at the proxy and the function is properly proxied through.
CVE-2023-30541 - MEDIUM - (@openzeppelin/contracts-upgradeable >=3.2.0 <4.8.3): OpenZeppelin Contracts is a library for secure smart contract development. A function in the implementation contract may be inaccessible if its selector clashes with one of the proxy's own selectors. Specifically, if the clashing function has a different signature with incompatible ABI encoding, the proxy could revert while attempting to decode the arguments from calldata. The probability of an accidental clash is negligible, but one could be caused deliberately and could cause a reduction in availability. The issue has been fixed in version 4.8.3. As a workaround if a function appears to be inaccessible for this reason, it may be possible to craft the calldata such that ABI decoding does not fail at the proxy and the function is properly proxied through.
There are 8 instances:
Constructors and initialization functions play a critical role in contracts by setting important initial states when the contract is first deployed before the system starts. The parameters passed to the constructor and initialization functions directly affect the behavior of the contract / protocol. If incorrect parameters are provided, the system may fail to run, behave abnormally, be unstable, or lack security. Therefore, it's crucial to carefully check each parameter in the constructor and initialization functions. If an exception is found, the transaction should be rolled back.
<details> <summary>There are 17 instances (click to show):</summary>/// @audit `_assertionHash` 31: constructor( 32: address _rollup, 33: bytes32 _assertionHash 34: ) AbsBoldStakingPool(IRollupCore(_rollup).stakeToken()) {
/// @audit `_edgeId` 35: constructor( 36: address _challengeManager, 37: bytes32 _edgeId 38: ) AbsBoldStakingPool(address(EdgeChallengeManager(_challengeManager).stakeToken())) {
/// @audit `_maxDataSize` /// @audit `_isUsingFeeToken` /// @audit `_isDelayBufferable` 140: constructor( 141: uint256 _maxDataSize, 142: IReader4844 reader4844_, 143: bool _isUsingFeeToken, 144: bool _isDelayBufferable 145: ) { /// @audit `maxTimeVariation_` /// @audit `bufferConfig_` 177: function initialize( 178: IBridge bridge_, 179: ISequencerInbox.MaxTimeVariation calldata maxTimeVariation_, 180: BufferConfig memory bufferConfig_ 181: ) external onlyDelegated {
/// @audit `layerZeroBlockEdgeHeight` /// @audit `layerZeroBigStepEdgeHeight` /// @audit `layerZeroSmallStepEdgeHeight` 305: function initialize( 306: IAssertionChain _assertionChain, 307: uint64 _challengePeriodBlocks, 308: IOneStepProofEntry _oneStepProofEntry, 309: uint256 layerZeroBlockEdgeHeight, 310: uint256 layerZeroBigStepEdgeHeight, 311: uint256 layerZeroSmallStepEdgeHeight, 312: IERC20 _stakeToken, 313: address _excessStakeReceiver, 314: uint8 _numBigStepLevel, 315: uint256[] calldata _stakeAmounts 316: ) public initializer {
/// @audit `__array` 169: constructor(uint256[] memory __array) { /// @audit `contracts` /// @audit `proxyAdmins` /// @audit `implementations` /// @audit `settings` 287: constructor( 288: Contracts memory contracts, 289: ProxyAdmins memory proxyAdmins, 290: Implementations memory implementations, 291: Settings memory settings 292: ) {
</details>/// @audit `_ethBasedTemplates` /// @audit `_erc20BasedTemplates` 45: constructor( 46: BridgeTemplates memory _ethBasedTemplates, 47: BridgeTemplates memory _erc20BasedTemplates 48: ) Ownable() {
Using the low-level calls of a solidity address can leave the contract open to gas grief attacks. These attacks occur when the called contract returns a large amount of data.
So when calling an external contract, it is necessary to check the length of the return data before reading/copying it (using returndatasize()).
There is 1 instance:
304: (bool sent, ) = msg.sender.call{value: address(this).balance}("");
Even if the function follows the best practice of check-effects-interaction, not using a reentrancy guard when there may be transfer hooks opens the users of this protocol up to read-only reentrancy vulnerability with no way to protect them except by block-listing the entire protocol.
<details> <summary>There are 9 instances (click to show):</summary>35: IERC20(stakeToken).safeTransferFrom(msg.sender, address(this), amount); 51: IERC20(stakeToken).safeTransfer(msg.sender, amount);
434: st.safeTransferFrom(msg.sender, receiver, sa); 581: st.safeTransfer(edge.staker, sa);
312: IERC20(_nativeToken).safeTransferFrom(msg.sender, _inbox, totalFee);
</details>215: IERC20(stakeToken).safeTransfer(loserStakeEscrow, assertion.beforeStateData.configData.requiredStake); 295: IERC20(stakeToken).safeTransfer(loserStakeEscrow, assertion.beforeStateData.configData.requiredStake); 362: IERC20(stakeToken).safeTransfer(msg.sender, amount); 367: IERC20(stakeToken).safeTransferFrom(msg.sender, address(this), tokenAmount);
It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user).
<details> <summary>There are 11 instances (click to show):</summary>894: function setIsBatchPoster(address addr, bool isBatchPoster_) 895: external 896: onlyRollupOwnerOrBatchPosterManager 897: { 933: function setIsSequencer(address addr, bool isSequencer_) 934: external 935: onlyRollupOwnerOrBatchPosterManager 936: { 942: function setBatchPosterManager(address newBatchPosterManager) external onlyRollupOwner {
411: function upgradeSurroundingContracts(address newRollupAddress) private {
53: function updateTemplates(BridgeTemplates calldata _newTemplates) external onlyOwner { 58: function updateERC20Templates(BridgeTemplates calldata _newTemplates) external onlyOwner {
195: function setOwner(address newOwner) external override {
355: function deleteStaker(address stakerAddress) private {
63: function setTemplates( 64: BridgeCreator _bridgeCreator, 65: IOneStepProofEntry _osp, 66: IEdgeChallengeManager _challengeManagerLogic, 67: IRollupAdmin _rollupAdminLogic, 68: IRollupUser _rollupUserLogic, 69: IUpgradeExecutor _upgradeExecutorLogic, 70: address _validatorWalletCreator, 71: DeployHelper _l2FactoriesDeployer 72: ) external onlyOwner {
</details>232: function _addToDeposit(address stakerAddress, uint256 depositAmount) internal onlyValidator whenNotPaused { 349: function addToDeposit(address stakerAddress, uint256 tokenAmount) external onlyValidator whenNotPaused {
When data is pushed into the array, it does not overwrite or remove the existing identical elements. Duplicate data added to the state array can cause problems such as operations being repeated incorrectly, assets being assigned repeatedly or weights being calculated repeatedly.
There is 1 instance:
/// @audit createNewStake() 276: _stakerList.push(stakerAddress);
Code should follow the best-practice of check-effects-interaction, where state variables are updated before any external calls are made. Doing so prevents a large class of reentrancy bugs.
<details> <summary>There are 32 instances (click to show):</summary>/// @audit `stakeToken()` is called on line 38 39: challengeManager = _challengeManager; /// @audit `stakeToken()` is called on line 38 40: edgeId = _edgeId;
/// @audit `delayedMessageCount()` is called on line 807 816: totalDelayedMessagesRead = afterDelayedMessagesRead;
/// @audit `pause()` is called on line 342 348: stakerCount = 50; /// @audit `pause()` is called on line 342 350: for (uint64 i = 0; i < stakerCount; i++) { /// @audit `pause()` is called on line 342 355: stakersToRefund[0] = stakerAddr; /// @audit `cleanupOldRollup()` is called on line 466, triggering an external call - `pause()` 522: config.owner = address(this); /// @audit `cleanupOldRollup()` is called on line 466, triggering an external call - `pause()` 528: for (uint256 i = 0; i < validators.length; i++) { /// @audit `cleanupOldRollup()` is called on line 466, triggering an external call - `pause()` 530: _vals[i] = true;
/// @audit `setDelayedInbox()` is called on line 26 29: inbox = connectedContracts.inbox; /// @audit `setDelayedInbox()` is called on line 26 30: outbox = connectedContracts.outbox; /// @audit `setDelayedInbox()` is called on line 26 32: rollupEventInbox = connectedContracts.rollupEventInbox; /// @audit `setDelayedInbox()` is called on line 26 44: validatorWalletCreator = connectedContracts.validatorWalletCreator; /// @audit `setDelayedInbox()` is called on line 26 45: challengeManager = connectedContracts.challengeManager; /// @audit `setDelayedInbox()` is called on line 26 47: confirmPeriodBlocks = config.confirmPeriodBlocks; /// @audit `setDelayedInbox()` is called on line 26 48: chainId = config.chainId; /// @audit `setDelayedInbox()` is called on line 26 49: baseStake = config.baseStake; /// @audit `setDelayedInbox()` is called on line 26 50: wasmModuleRoot = config.wasmModuleRoot; /// @audit `setDelayedInbox()` is called on line 26 52: minimumAssertionPeriod = 75; /// @audit `setDelayedInbox()` is called on line 26 53: challengeGracePeriodBlocks = config.challengeGracePeriodBlocks; /// @audit `setDelayedInbox()` is called on line 26 58: loserStakeEscrow = config.loserStakeEscrow; /// @audit `setDelayedInbox()` is called on line 26 60: stakeToken = config.stakeToken; /// @audit `setDelayedInbox()` is called on line 26 61: anyTrustFastConfirmer = config.anyTrustFastConfirmer; /// @audit `setDelayedInbox()` is called on line 26 74: currentInboxCount += 1; /// @audit `setDelayedInbox()` is called on line 26 89: assertionInputs.afterState = config.genesisAssertionState; /// @audit `setDelayedInbox()` is called on line 26 102: _assertionCreatedAtArbSysBlock[genesisHash] = ArbSys(address(100)).arbBlockNumber();
/// @audit `updateSendRoot()` is called on line 261 263: _latestConfirmed = assertionHash; /// @audit `updateSendRoot()` is called on line 261 264: assertion.status = AssertionStatus.Confirmed;
</details>/// @audit `()` is called on line 188 208: deployParams.config.owner = address(this); /// @audit `()` is called on line 188 225: for (uint256 i = 0; i < deployParams.batchPosters.length; i++) { /// @audit `()` is called on line 188 235: for (uint256 i = 0; i < deployParams.validators.length; i++) { /// @audit `()` is called on line 188 236: _vals[i] = true;
Using import declarations of the form import {<identifier_name>} from "some/file.sol" avoids polluting the symbol namespace making flattened files smaller, and speeds up compilation (but does not save any gas).
8: import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; 9: import "../rollup/IRollupLogic.sol"; 10: import "./AbsBoldStakingPool.sol"; 11: import "./interfaces/IAssertionStakingPool.sol";
8: import "./AssertionStakingPool.sol"; 9: import "./StakingPoolCreatorUtils.sol"; 10: import "./interfaces/IAssertionStakingPoolCreator.sol";
7: import "./AbsBoldStakingPool.sol"; 8: import "./interfaces/IEdgeStakingPool.sol"; 9: import "../challengeV2/EdgeChallengeManager.sol"; 11: import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; 12: import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
8: import "./EdgeStakingPool.sol"; 9: import "./StakingPoolCreatorUtils.sol"; 10: import "./interfaces/IEdgeStakingPoolCreator.sol";
8: import "@openzeppelin/contracts/utils/Create2.sol"; 9: import "@openzeppelin/contracts/utils/Address.sol";
7: import "../../rollup/IRollupLogic.sol"; 8: import "./IAbsBoldStakingPool.sol";
7: import "../../rollup/IRollupLogic.sol"; 8: import "./IAssertionStakingPool.sol";
7: import "../../challengeV2/EdgeChallengeManager.sol"; 8: import "./IAbsBoldStakingPool.sol";
7: import "./IEdgeStakingPool.sol";
7: import "./Messages.sol"; 8: import "./DelayBufferTypes.sol";
5: import "./Messages.sol";
9: import "../libraries/IGasRefunder.sol"; 10: import "./IDelayedMessageProvider.sol"; 11: import "./IBridge.sol"; 12: import "./Messages.sol"; 13: import "./DelayBufferTypes.sol";
40: import "./IBridge.sol"; 41: import "./IInboxBase.sol"; 42: import "./ISequencerInbox.sol"; 43: import "../rollup/IRollupLogic.sol"; 44: import "./Messages.sol"; 45: import "../precompiles/ArbGasInfo.sol"; 46: import "../precompiles/ArbSys.sol"; 47: import "../libraries/IReader4844.sol"; 50: import "../libraries/DelegateCallAware.sol"; 53: import "../libraries/ArbitrumChecker.sol"; 55: import "./DelayBuffer.sol";
7: import "../rollup/Assertion.sol"; 8: import "./libraries/UintUtilsLib.sol"; 9: import "./IAssertionChain.sol"; 10: import "./libraries/EdgeChallengeManagerLib.sol"; 11: import "../libraries/Constants.sol"; 12: import "../state/Machine.sol"; 14: import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; 15: import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
7: import "../bridge/IBridge.sol"; 8: import "../osp/IOneStepProofEntry.sol"; 9: import "../rollup/Assertion.sol";
7: import "./Enums.sol"; 8: import "./ChallengeErrors.sol";
7: import "./Enums.sol";
7: import "./UintUtilsLib.sol"; 8: import "./MerkleTreeLib.sol"; 9: import "./ChallengeEdgeLib.sol"; 10: import "../../osp/IOneStepProofEntry.sol"; 11: import "../../rollup/AssertionState.sol"; 12: import "../../libraries/Constants.sol"; 13: import "./ChallengeErrors.sol";
7: import "../../libraries/MerkleLib.sol"; 8: import "./ArrayUtilsLib.sol"; 9: import "./UintUtilsLib.sol";
7: import "./AssertionState.sol";
7: import "../state/GlobalState.sol"; 8: import "../state/Machine.sol"; 9: import "../osp/IOneStepProofEntry.sol";
7: import "@openzeppelin/contracts-upgradeable/utils/Create2Upgradeable.sol"; 8: import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; 9: import "./RollupProxy.sol"; 10: import "./RollupLib.sol"; 11: import "./RollupAdminLogic.sol";
7: import "../bridge/Bridge.sol"; 8: import "../bridge/SequencerInbox.sol"; 9: import "../bridge/Inbox.sol"; 10: import "../bridge/Outbox.sol"; 11: import "./RollupEventInbox.sol"; 12: import "../bridge/ERC20Bridge.sol"; 13: import "../bridge/ERC20Inbox.sol"; 14: import "../rollup/ERC20RollupEventInbox.sol"; 15: import "../bridge/ERC20Outbox.sol"; 17: import "../bridge/IBridge.sol"; 18: import "@openzeppelin/contracts/access/Ownable.sol"; 19: import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
7: import "../state/GlobalState.sol"; 8: import "../state/Machine.sol"; 9: import "../bridge/ISequencerInbox.sol"; 10: import "../bridge/IBridge.sol"; 11: import "../bridge/IOutbox.sol"; 12: import "../bridge/IInboxBase.sol"; 13: import "./IRollupEventInbox.sol"; 14: import "./IRollupLogic.sol"; 15: import "../challengeV2/EdgeChallengeManager.sol";
7: import "./IRollupCore.sol"; 8: import "../bridge/ISequencerInbox.sol"; 9: import "../bridge/IOutbox.sol"; 10: import "../bridge/IOwnable.sol"; 11: import "./Config.sol";
7: import "./Assertion.sol"; 8: import "../bridge/IBridge.sol"; 9: import "../bridge/IOutbox.sol"; 10: import "../bridge/IInboxBase.sol"; 11: import "./IRollupEventInbox.sol"; 12: import "../challengeV2/EdgeChallengeManager.sol"; 13: import "../challengeV2/IAssertionChain.sol";
7: import "./IRollupCore.sol"; 8: import "../bridge/ISequencerInbox.sol"; 9: import "../bridge/IOutbox.sol"; 10: import "../bridge/IOwnable.sol";
7: import "./IRollupAdmin.sol"; 8: import "./IRollupLogic.sol"; 9: import "./RollupCore.sol"; 10: import "../bridge/IOutbox.sol"; 11: import "../bridge/ISequencerInbox.sol"; 12: import "../libraries/DoubleLogicUUPSUpgradeable.sol"; 13: import "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol";
7: import "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol"; 9: import "./Assertion.sol"; 10: import "./RollupLib.sol"; 11: import "./IRollupEventInbox.sol"; 12: import "./IRollupCore.sol"; 14: import "../state/Machine.sol"; 16: import "../bridge/ISequencerInbox.sol"; 17: import "../bridge/IBridge.sol"; 18: import "../bridge/IOutbox.sol"; 19: import "../challengeV2/EdgeChallengeManager.sol"; 20: import "../libraries/ArbitrumChecker.sol";
7: import "./RollupProxy.sol"; 8: import "./IRollupAdmin.sol"; 9: import "./BridgeCreator.sol"; 10: import "@offchainlabs/upgrade-executor/src/IUpgradeExecutor.sol"; 11: import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; 12: import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; 13: import "@openzeppelin/contracts/access/Ownable.sol";
7: import "../state/GlobalState.sol"; 8: import "../bridge/ISequencerInbox.sol"; 10: import "../bridge/IBridge.sol"; 11: import "../bridge/IOutbox.sol"; 12: import "../bridge/IInboxBase.sol"; 13: import "./Assertion.sol"; 14: import "./IRollupEventInbox.sol"; 15: import "../challengeV2/EdgeChallengeManager.sol";
7: import "../libraries/AdminFallbackProxy.sol"; 8: import "./IRollupAdmin.sol"; 9: import "./Config.sol";
</details>7: import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; 10: import "../libraries/UUPSNotUpgradeable.sol"; 11: import "./RollupCore.sol"; 12: import "./IRollupLogic.sol";
To avoid misunderstandings and unexpected state accesses, it is recommended to explicitly define the visibility of each state variable.
There is 1 instance:
167: uint256[] _array;
private/internal functions should be prefixed with an underscoreIt is recommended by the Solidity Style Guide.
<details> <summary>There are 99 instances (click to show):</summary>13: function getPool(bytes memory creationCode, bytes memory args) internal view returns (address) {
33: function calcBuffer( 34: uint256 start, 35: uint256 end, 36: uint256 buffer, 37: uint256 sequenced, 38: uint256 threshold, 39: uint256 max, 40: uint256 replenishRateInBasis 41: ) internal pure returns (uint256) { 68: function update(BufferData storage self, uint64 blockNumber) internal { 82: function calcPendingBuffer(BufferData storage self, uint64 blockNumber) 83: internal 84: view 85: returns (uint64) 104: function isSynced(BufferData storage self) internal view returns (bool) { 108: function isUpdatable(BufferData storage self) internal view returns (bool) { 115: function isValidBufferConfig(BufferConfig memory config) internal pure returns (bool) {
216: function getTimeBounds() internal view virtual returns (IBridge.TimeBounds memory) { 269: function maxTimeVariationInternal() 270: internal 271: view 272: returns ( 273: uint64, 274: uint64, 275: uint64, 276: uint64 277: ) 455: function addSequencerL2BatchFromBlobsImpl( 456: uint256 sequenceNumber, 457: uint256 afterDelayedMessagesRead, 458: uint256 prevMessageCount, 459: uint256 newMessageCount 460: ) internal { 512: function addSequencerL2BatchFromCalldataImpl( 513: uint256 sequenceNumber, 514: bytes calldata data, 515: uint256 afterDelayedMessagesRead, 516: uint256 prevMessageCount, 517: uint256 newMessageCount, 518: bool isFromOrigin 519: ) internal { 605: function delayProofImpl(uint256 afterDelayedMessagesRead, DelayProof memory delayProof) 606: internal 607: { 628: function isDelayProofRequired(uint256 afterDelayedMessagesRead) internal view returns (bool) { 637: function packHeader(uint256 afterDelayedMessagesRead) 638: internal 639: view 640: returns (bytes memory, IBridge.TimeBounds memory) 659: function formEmptyDataHash(uint256 afterDelayedMessagesRead) 660: internal 661: view 662: returns (bytes32, IBridge.TimeBounds memory) 675: function isValidCallDataFlag(bytes1 headerByte) internal pure returns (bool) { 688: function formCallDataHash(bytes calldata data, uint256 afterDelayedMessagesRead) 689: internal 690: view 691: returns (bytes32, IBridge.TimeBounds memory) 725: function formBlobDataHash(uint256 afterDelayedMessagesRead) 726: internal 727: view 728: virtual 729: returns ( 730: bytes32, 731: IBridge.TimeBounds memory, 732: uint256 733: ) 755: function submitBatchSpendingReport( 756: bytes32 dataHash, 757: uint256 seqMessageIndex, 758: uint256 gasPrice, 759: uint256 extraGas 760: ) internal { 791: function addSequencerL2BatchImpl( 792: bytes32 dataHash, 793: uint256 afterDelayedMessagesRead, 794: uint256 calldataLengthPosted, 795: uint256 prevMessageCount, 796: uint256 newMessageCount 797: ) 798: internal 799: returns ( 800: uint256 seqMessageIndex, 801: bytes32 beforeAcc, 802: bytes32 delayedAcc, 803: bytes32 acc 804: ) 843: function delayBufferableBlocks(uint64 _buffer) internal view returns (uint64) {
13: function append(bytes32[] memory arr, bytes32 newItem) internal pure returns (bytes32[] memory) { 27: function slice(bytes32[] memory arr, uint256 startIndex, uint256 endIndex) 28: internal 29: pure 30: returns (bytes32[] memory) 45: function concat(bytes32[] memory arr1, bytes32[] memory arr2) internal pure returns (bytes32[] memory) {
69: function newEdgeChecks( 70: bytes32 originId, 71: bytes32 startHistoryRoot, 72: uint256 startHeight, 73: bytes32 endHistoryRoot, 74: uint256 endHeight 75: ) internal pure { 93: function newLayerZeroEdge( 94: bytes32 originId, 95: bytes32 startHistoryRoot, 96: uint256 startHeight, 97: bytes32 endHistoryRoot, 98: uint256 endHeight, 99: bytes32 claimId, 100: address staker, 101: uint8 level 102: ) internal view returns (ChallengeEdge memory) { 133: function newChildEdge( 134: bytes32 originId, 135: bytes32 startHistoryRoot, 136: uint256 startHeight, 137: bytes32 endHistoryRoot, 138: uint256 endHeight, 139: uint8 level 140: ) internal view returns (ChallengeEdge memory) { 166: function mutualIdComponent( 167: uint8 level, 168: bytes32 originId, 169: uint256 startHeight, 170: bytes32 startHistoryRoot, 171: uint256 endHeight 172: ) internal pure returns (bytes32) { 180: function mutualId(ChallengeEdge storage ce) internal view returns (bytes32) { 184: function mutualIdMem(ChallengeEdge memory ce) internal pure returns (bytes32) { 189: function idComponent( 190: uint8 level, 191: bytes32 originId, 192: uint256 startHeight, 193: bytes32 startHistoryRoot, 194: uint256 endHeight, 195: bytes32 endHistoryRoot 196: ) internal pure returns (bytes32) { 208: function idMem(ChallengeEdge memory edge) internal pure returns (bytes32) { 215: function id(ChallengeEdge storage edge) internal view returns (bytes32) { 222: function exists(ChallengeEdge storage edge) internal view returns (bool) { 228: function length(ChallengeEdge storage edge) internal view returns (uint256) { 239: function setChildren(ChallengeEdge storage edge, bytes32 lowerChildId, bytes32 upperChildId) internal { 249: function setConfirmed(ChallengeEdge storage edge) internal { 258: function isLayerZero(ChallengeEdge storage edge) internal view returns (bool) { 264: function setRefunded(ChallengeEdge storage edge) internal { 279: function levelToType(uint8 level, uint8 numBigStepLevels) internal pure returns (EdgeType eType) {
141: function get(EdgeStore storage store, bytes32 edgeId) internal view returns (ChallengeEdge storage) { 152: function getNoCheck(EdgeStore storage store, bytes32 edgeId) internal view returns (ChallengeEdge storage) { 160: function add(EdgeStore storage store, ChallengeEdge memory edge) internal returns (EdgeAddedData memory) { 213: function layerZeroTypeSpecificChecks( 214: EdgeStore storage store, 215: CreateEdgeArgs calldata args, 216: AssertionReferenceData memory ard, 217: IOneStepProofEntry oneStepProofEntry, 218: uint8 numBigStepLevel 219: ) private view returns (ProofData memory, bytes32) { 327: function isPowerOfTwo(uint256 x) internal pure returns (bool) { 344: function layerZeroCommonChecks(ProofData memory proofData, CreateEdgeArgs calldata args, uint256 expectedEndHeight) 345: private 346: pure 347: returns (bytes32) 391: function toLayerZeroEdge(bytes32 originId, bytes32 startHistoryRoot, CreateEdgeArgs calldata args) 392: private 393: view 394: returns (ChallengeEdge memory) 411: function createLayerZeroEdge( 412: EdgeStore storage store, 413: CreateEdgeArgs calldata args, 414: AssertionReferenceData memory ard, 415: IOneStepProofEntry oneStepProofEntry, 416: uint256 expectedEndHeight, 417: uint8 numBigStepLevel, 418: bool whitelistEnabled 419: ) internal returns (EdgeAddedData memory) { 443: function getPrevAssertionHash(EdgeStore storage store, bytes32 edgeId) internal view returns (bytes32) { 463: function hasRival(EdgeStore storage store, bytes32 edgeId) internal view returns (bool) { 483: function hasLengthOneRival(EdgeStore storage store, bytes32 edgeId) internal view returns (bool) { 488: function timeUnrivaledTotal(EdgeStore storage store, bytes32 edgeId) internal view returns (uint256) { 502: function updateTimerCache(EdgeStore storage store, bytes32 edgeId, uint256 newValue) 503: internal 504: returns (bool, uint256) 516: function updateTimerCacheByChildren(EdgeStore storage store, bytes32 edgeId) internal returns (bool, uint256) { 520: function updateTimerCacheByClaim( 521: EdgeStore storage store, 522: bytes32 edgeId, 523: bytes32 claimingEdgeId, 524: uint8 numBigStepLevel 525: ) internal returns (bool, uint256) { 537: function timeUnrivaled(EdgeStore storage store, bytes32 edgeId) internal view returns (uint256) { 576: function mandatoryBisectionHeight(uint256 start, uint256 end) internal pure returns (uint256) { 604: function bisectEdge(EdgeStore storage store, bytes32 edgeId, bytes32 bisectionHistoryRoot, bytes memory prefixProof) 605: internal 606: returns (bytes32, EdgeAddedData memory, EdgeAddedData memory) 662: function setConfirmedRival(EdgeStore storage store, bytes32 edgeId) internal { 674: function nextEdgeLevel(uint8 level, uint8 numBigStepLevel) internal pure returns (uint8) { 689: function checkClaimIdLink(EdgeStore storage store, bytes32 edgeId, bytes32 claimingEdgeId, uint8 numBigStepLevel) 690: private 691: view 692: { 723: function confirmEdgeByTime( 724: EdgeStore storage store, 725: bytes32 edgeId, 726: uint64 claimedAssertionUnrivaledBlocks, 727: uint64 confirmationThresholdBlock 728: ) internal returns (uint256) { 766: function confirmEdgeByOneStepProof( 767: EdgeStore storage store, 768: bytes32 edgeId, 769: IOneStepProofEntry oneStepProofEntry, 770: OneStepData calldata oneStepData, 771: ExecutionContext memory execCtx, 772: bytes32[] calldata beforeHistoryInclusionProof, 773: bytes32[] calldata afterHistoryInclusionProof, 774: uint8 numBigStepLevel, 775: uint256 bigStepHeight, 776: uint256 smallStepHeight 777: ) internal {
110: function root(bytes32[] memory me) internal pure returns (bytes32) { 151: function appendCompleteSubTree(bytes32[] memory me, uint256 level, bytes32 subtreeRoot) 152: internal 153: pure 154: returns (bytes32[] memory) 238: function appendLeaf(bytes32[] memory me, bytes32 leaf) internal pure returns (bytes32[] memory) { 251: function maximumAppendBetween(uint256 startSize, uint256 endSize) internal pure returns (uint256) { 292: function treeSize(bytes32[] memory me) internal pure returns (uint256) { 312: function verifyPrefixProof( 313: bytes32 preRoot, 314: uint256 preSize, 315: bytes32 postRoot, 316: uint256 postSize, 317: bytes32[] memory preExpansion, 318: bytes32[] memory proof 319: ) internal pure { 359: function verifyInclusionProof(bytes32 rootHash, bytes32 leaf, uint256 index, bytes32[] memory proof) 360: internal 361: pure 362: {
14: function leastSignificantBit(uint256 x) internal pure returns (uint256 msb) { 29: function mostSignificantBit(uint256 x) internal pure returns (uint256 msb) {
70: function createAssertion( 71: bool _isFirstChild, 72: bytes32 _configHash 73: ) internal view returns (AssertionNode memory) { 85: function childCreated(AssertionNode storage self) internal { 93: function requireExists(AssertionNode memory self) internal pure {
18: function toExecutionState(AssertionState memory state) internal pure returns (ExecutionState memory) { 22: function hash(AssertionState memory state) internal pure returns (bytes32) {
341: function cleanupOldRollup() private { 367: function createConfig() private view returns (Config memory) { 411: function upgradeSurroundingContracts(address newRollupAddress) private { 433: function upgradeSequencerInbox() private {
126: function getAssertionStorage(bytes32 assertionHash) internal view returns (AssertionNode storage) { 225: function initializeCore(AssertionNode memory initialAssertion, bytes32 assertionHash) internal { 236: function confirmAssertionInternal( 237: bytes32 assertionHash, 238: bytes32 parentAssertionHash, 239: AssertionState calldata confirmState, 240: bytes32 inboxAcc 241: ) internal { 274: function createNewStake(address stakerAddress, uint256 depositAmount, address withdrawalAddress) internal { 286: function increaseStakeBy(address stakerAddress, uint256 amountAdded) internal { 300: function reduceStakeTo(address stakerAddress, uint256 target) internal returns (uint256) { 317: function withdrawStaker(address stakerAddress) internal { 331: function withdrawFunds(address account) internal returns (uint256) { 343: function increaseWithdrawableFunds(address account, uint256 amount) internal { 355: function deleteStaker(address stakerAddress) private { 365: function createNewAssertion( 366: AssertionInputs calldata assertion, 367: bytes32 prevAssertionHash, 368: bytes32 expectedAssertionHash 369: ) internal returns (bytes32 newAssertionHash, bool overflowAssertion) { 565: function requireInactiveStaker(address stakerAddress) internal view {
85: function createChallengeManager(address rollupAddr, address proxyAdminAddr, Config memory config) 86: internal 87: returns (IEdgeChallengeManager)
23: function assertionHash( 24: bytes32 parentAssertionHash, 25: AssertionState memory afterState, 26: bytes32 inboxAcc 27: ) internal pure returns (bytes32) { 37: function assertionHash( 38: bytes32 parentAssertionHash, 39: bytes32 afterStateHash, 40: bytes32 inboxAcc 41: ) internal pure returns (bytes32) { 54: function configHash( 55: bytes32 wasmModuleRoot, 56: uint256 requiredStake, 57: address challengeManager, 58: uint64 confirmPeriodBlocks, 59: uint64 nextInboxPosition 60: ) internal pure returns (bytes32) { 73: function validateConfigHash( 74: ConfigData calldata configData, 75: bytes32 _configHash 76: ) internal pure {
</details>366: function receiveTokens(uint256 tokenAmount) private {
private/internal state variables should be prefixed with an underscoreIt is recommended by the Solidity Style Guide.
<details> <summary>There are 10 instances (click to show):</summary>91: uint256 internal constant GAS_PER_BLOB = 1 << 17; 119: uint64 internal delayBlocks; 120: uint64 internal futureBlocks; 121: uint64 internal delaySeconds; 122: uint64 internal futureSeconds; 129: uint256 internal immutable deployTimeChainId = block.chainid; 131: bool internal immutable hostChainIsArbitrum = ArbitrumChecker.runningOnArbitrum();
266: EdgeStore internal store;
99: mapping(bytes32 => bytes) internal preImages;
</details>31: uint256 internal immutable deployTimeChainId = block.chainid;
override is unnecessaryStarting from Solidity 0.8.8, the override keyword is not required when overriding an interface function, except for the case where the function is defined in multiple bases.
<details> <summary>There are 33 instances (click to show):</summary>560: function addSequencerL2Batch( 561: uint256 sequenceNumber, 562: bytes calldata data, 563: uint256 afterDelayedMessagesRead, 564: IGasRefunder gasRefunder, 565: uint256 prevMessageCount, 566: uint256 newMessageCount 567: ) external override refundsGas(gasRefunder, IReader4844(address(0))) {
18: function initialize(Config calldata config, ContractDependencies calldata connectedContracts) 19: external 20: override 21: onlyProxy 22: initializer 23: { 117: function setOutbox(IOutbox _outbox) external override { 127: function removeOldOutbox(address _outbox) external override { 138: function setDelayedInbox(address _inbox, bool _enabled) external override { 150: function pause() external override { 158: function resume() external override { 166: function _authorizeUpgrade(address newImplementation) internal override {} 171: function _authorizeSecondaryUpgrade(address newImplementation) internal override {} 180: function setValidator(address[] calldata _validator, bool[] calldata _val) external override { 195: function setOwner(address newOwner) external override { 204: function setMinimumAssertionPeriod(uint256 newPeriod) external override { 213: function setConfirmPeriodBlocks(uint64 newConfirmPeriod) external override { 223: function setBaseStake(uint256 newBaseStake) external override { 228: function forceRefundStaker(address[] calldata staker) external override whenPaused { 237: function forceCreateAssertion( 238: bytes32 prevAssertionHash, 239: AssertionInputs calldata assertion, 240: bytes32 expectedAssertionHash 241: ) external override whenPaused { 258: function forceConfirmAssertion( 259: bytes32 assertionHash, 260: bytes32 parentAssertionHash, 261: AssertionState calldata confirmState, 262: bytes32 inboxAcc 263: ) external override whenPaused { 269: function setLoserStakeEscrow(address newLoserStakerEscrow) external override { 281: function setWasmModuleRoot(bytes32 newWasmModuleRoot) external override { 290: function setSequencerInbox(address _sequencerInbox) external override {
134: function getAssertion(bytes32 assertionHash) public view override returns (AssertionNode memory) { 145: function getAssertionCreationBlockForLogLookup(bytes32 assertionHash) external view override returns (uint256) { 162: function getStakerAddress(uint64 stakerNum) external view override returns (address) { 171: function isStaked(address staker) public view override returns (bool) { 180: function latestStakedAssertion(address staker) public view override returns (bytes32) { 189: function amountStaked(address staker) public view override returns (uint256) { 198: function getStaker(address staker) external view override returns (Staker memory) { 207: function withdrawableFunds(address user) external view override returns (uint256) { 212: function latestConfirmed() public view override returns (bytes32) { 217: function stakerCount() public view override returns (uint64) {
</details>27: function initialize(address _stakeToken) external view override onlyProxy { 222: function returnOldDeposit() external override onlyValidator whenNotPaused { 358: function withdrawStakerFunds() external override whenNotPaused returns (uint256) {
function func(address a, address) -> function func(address a, address /* b */)
There are 5 instances:
95: function isBatchPoster(address) external view returns (bool); 97: function isSequencer(address) external view returns (bool); 124: function dasKeySetInfo(bytes32) external view returns (bool, uint64);
356: function addSequencerL2BatchFromOrigin( 357: uint256, 358: bytes calldata, 359: uint256, 360: IGasRefunder 361: ) external pure {
26: function isValidator(address) external view returns (bool);
While the compiler automatically provides a getter for accessing single elements within a public state variable array, it doesn't provide a way to fetch the whole array, or subsets thereof. Consider adding a function to allow the fetching of slices of the array, especially if the contract doesn't already have multicall functionality.
There is 1 instance:
279: uint256[] public stakeAmounts;
Assign the expression's parts to intermediate local variables, and check against those instead.
There is 1 instance:
16: _getAdmin() == address(0) && 17: _getImplementation() == address(0) && 18: _getSecondaryImplementation() == address(0)
bytes for clearer semantic meaningUsing a cast on a single argument, rather than abi.encodePacked() makes the intended operation more clear, leading to less reviewer confusion.
There is 1 instance:
135: bytes32 public constant UNRIVALED = keccak256(abi.encodePacked("UNRIVALED"));
Unless the code is attempting to delete the state variable, the caller shouldn't have to write "" to the state variable
There are 4 instances:
36: assertionHash = _assertionHash;
40: edgeId = _edgeId;
282: wasmModuleRoot = newWasmModuleRoot;
229: _latestConfirmed = assertionHash;
Doing so will significantly increase centralization, but will help to prevent hackers from using stolen tokens
There are 4 instances:
16: abstract contract AbsBoldStakingPool is IAbsBoldStakingPool {
206: contract EdgeChallengeManager is IEdgeChallengeManager, Initializable {
17: contract RollupCreator is Ownable {
15: contract RollupUserLogic is RollupCore, UUPSNotUpgradeable, IRollupUser {
Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants/immutables in a single location is to create an internal constant in a library. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync.
There are 4 instances:
/// @audit Seen in src/rollup/BOLDUpgradeAction.sol#124 25: address public immutable rollup;
/// @audit Seen in src/rollup/RollupUserLogic.sol#31 129: uint256 internal immutable deployTimeChainId = block.chainid;
/// @audit Seen in src/assertionStakingPool/AssertionStakingPool.sol#25 124: IOldRollup public immutable rollup;
/// @audit Seen in src/bridge/SequencerInbox.sol#129 31: uint256 internal immutable deployTimeChainId = block.chainid;
if-statements to ternary expressionsConverting some if statements to ternaries (such as z = (a < b) ? x : y) can make the code more concise and easier to read.
There are 2 instances:
129: } else if (val != 0) { 130: // accum represents the smaller sub trees, since it is earlier in the expansion 131: // we put the larger subtrees on the left 132: accum = keccak256(abi.encodePacked(val, accum)); 133: } else { 134: // by definition we always complete trees by appending zeros to the right 135: accum = keccak256(abi.encodePacked(accum, bytes32(0))); 136: }
451: if (afterInboxPosition == currentInboxPosition) { 452: // No new messages have been added to the inbox since the last assertion 453: // In this case if we set the next inbox position to the current one we would be insisting that 454: // the next assertion process no messages. So instead we increment the next inbox position to current 455: // plus one, so that the next assertion will process exactly one message. 456: // Thus, no assertion can be empty (except the genesis assertion, which is created 457: // via a different codepath). 458: nextInboxPosition = currentInboxPosition + 1; 459: } else { 460: nextInboxPosition = currentInboxPosition; 461: }
Keeping each contract in a separate file makes it easier to work with multiple people, makes the code easier to maintain, and is a common practice on most projects. The following files each contains more than one contract/library/interface.
There are 2 instances:
According to the Solidity Style Guide, contract and library names should also match their filenames.
There are 3 instances:
/// @audit Not match filename `Assertion.sol` 66: library AssertionNodeLib {
/// @audit Not match filename `AssertionState.sol` 17: library AssertionStateLib {
/// @audit Not match filename `IRollupLogic.sol` 12: interface IRollupUser is IRollupCore, IOwnable {
constant/immutable variablesIf the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).
There are 5 instances:
99: ISequencerInbox.MaxTimeVariation private __LEGACY_MAX_TIME_VARIATION;
291: uint256 public LAYERZERO_BLOCKEDGE_HEIGHT; 293: uint256 public LAYERZERO_BIGSTEPEDGE_HEIGHT; 295: uint256 public LAYERZERO_SMALLSTEPEDGE_HEIGHT; 298: uint8 public NUM_BIGSTEP_LEVEL;
This will allow users to easily exactly pinpoint when and by whom a contract was constructed.
<details> <summary>There are 10 instances (click to show):</summary>24: constructor(address _stakeToken) {
31: constructor( 32: address _rollup, 33: bytes32 _assertionHash 34: ) AbsBoldStakingPool(IRollupCore(_rollup).stakeToken()) {
35: constructor( 36: address _challengeManager, 37: bytes32 _edgeId 38: ) AbsBoldStakingPool(address(EdgeChallengeManager(_challengeManager).stakeToken())) {
140: constructor( 141: uint256 _maxDataSize, 142: IReader4844 reader4844_, 143: bool _isUsingFeeToken, 144: bool _isDelayBufferable 145: ) {
300: constructor() {
126: constructor(IOldRollup _rollup) { 169: constructor(uint256[] memory __array) { 287: constructor( 288: Contracts memory contracts, 289: ProxyAdmins memory proxyAdmins, 290: Implementations memory implementations, 291: Settings memory settings 292: ) {
45: constructor( 46: BridgeTemplates memory _ethBasedTemplates, 47: BridgeTemplates memory _erc20BasedTemplates 48: ) Ownable() {
</details>58: constructor() Ownable() {}
Empty function body in solidity is not recommended, consider adding some comments to the body.
There are 3 instances:
166: function _authorizeUpgrade(address newImplementation) internal override {} 171: function _authorizeSecondaryUpgrade(address newImplementation) internal override {}
61: receive() external payable {}
When an action is triggered based on a user's action, not being able to filter based on who triggered the action makes event processing a lot more cumbersome. Including the msg.sender the events of these types of action will make events much more useful to end users, especially when msg.sender is not tx.origin.
20: emit NewAssertionPoolCreated(_rollup, _assertionHash, address(assertionPool));
20: emit NewEdgeStakingPoolCreated(challengeManager, edgeId);
344: emit SequencerBatchDelivered( 345: seqMessageIndex, 346: beforeAcc, 347: afterAcc, 348: delayedAcc, 349: totalDelayedMessagesRead, 350: timeBounds, 351: IBridge.BatchDataLocation.NoData 352: ); 488: emit SequencerBatchDelivered( 489: sequenceNumber, 490: beforeAcc, 491: afterAcc, 492: delayedAcc, 493: totalDelayedMessagesRead, 494: timeBounds, 495: IBridge.BatchDataLocation.Blob 496: );
437: emit EdgeAdded( 438: edgeAdded.edgeId, 439: edgeAdded.mutualId, 440: edgeAdded.originId, 441: edgeAdded.claimId, 442: edgeAdded.length, 443: edgeAdded.level, 444: edgeAdded.hasRival, 445: edgeAdded.isLayerZero 446: ); 462: emit EdgeAdded( 463: lowerChildAdded.edgeId, 464: lowerChildAdded.mutualId, 465: lowerChildAdded.originId, 466: lowerChildAdded.claimId, 467: lowerChildAdded.length, 468: lowerChildAdded.level, 469: lowerChildAdded.hasRival, 470: lowerChildAdded.isLayerZero 471: ); 474: emit EdgeAdded( 475: upperChildAdded.edgeId, 476: upperChildAdded.mutualId, 477: upperChildAdded.originId, 478: upperChildAdded.claimId, 479: upperChildAdded.length, 480: upperChildAdded.level, 481: upperChildAdded.hasRival, 482: upperChildAdded.isLayerZero 483: ); 485: emit EdgeBisected(edgeId, lowerChildId, upperChildAdded.edgeId, lowerChildAlreadyExists); 494: if (updated) emit TimerCacheUpdated(edgeIds[i], newValue); 501: if (updated) emit TimerCacheUpdated(edgeId, newValue); 507: if (updated) emit TimerCacheUpdated(edgeId, newValue); 535: emit EdgeConfirmedByTime(edgeId, store.edges[edgeId].mutualId(), totalTimeUnrivaled); 568: emit EdgeConfirmedByOneStepProof(edgeId, store.edges[edgeId].mutualId()); 584: emit EdgeRefunded(edgeId, store.edges[edgeId].mutualId(), address(st), sa);
109: emit HashSet(h, executionState, inboxMaxCount);
</details>120: emit OwnerFunctionCalled(0); 130: emit OwnerFunctionCalled(1); 140: emit OwnerFunctionCalled(2); 152: emit OwnerFunctionCalled(3); 160: emit OwnerFunctionCalled(4); 187: emit OwnerFunctionCalled(6); 206: emit OwnerFunctionCalled(8); 216: emit OwnerFunctionCalled(9); 225: emit OwnerFunctionCalled(12); 234: emit OwnerFunctionCalled(22); 255: emit OwnerFunctionCalled(23); 266: emit OwnerFunctionCalled(24); 274: emit OwnerFunctionCalled(25); 283: emit OwnerFunctionCalled(26); 292: emit OwnerFunctionCalled(27); 301: emit OwnerFunctionCalled(28); 310: emit OwnerFunctionCalled(30); 319: emit OwnerFunctionCalled(31); 328: emit OwnerFunctionCalled(32);
Source files are using different floating version syntax, this is prone to compilation errors, and is not conducive to the code reliability and maintainability.
There are 4 instances:
5: pragma solidity ^0.8.0;
7: pragma solidity >=0.6.9 <0.9.0;
5: pragma solidity ^0.8.17;
5: pragma solidity ^0.8.4;
Function state mutability can be restricted to pure
There are 2 instances:
166: function _authorizeUpgrade(address newImplementation) internal override {} 171: function _authorizeSecondaryUpgrade(address newImplementation) internal override {}
The contract's interface should be imported first, followed by each of the interfaces it uses, followed by all other files. The examples below do not follow this layout.
<details> <summary>There are 28 instances (click to show):</summary>/// @audit Out of order with the prev import️ ⬆ 9: import {IAbsBoldStakingPool} from "./interfaces/IAbsBoldStakingPool.sol";
/// @audit Out of order with the prev import️ ⬆ 11: import "./interfaces/IAssertionStakingPool.sol";
/// @audit Out of order with the prev import️ ⬆ 10: import "./interfaces/IAssertionStakingPoolCreator.sol";
/// @audit Out of order with the prev import️ ⬆ 8: import "./interfaces/IEdgeStakingPool.sol"; /// @audit Out of order with the prev import️ ⬆ 11: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
/// @audit Out of order with the prev import️ ⬆ 10: import "./interfaces/IEdgeStakingPoolCreator.sol";
/// @audit Out of order with the prev import️ ⬆ 8: import "./IAbsBoldStakingPool.sol";
/// @audit Out of order with the prev import️ ⬆ 40: import "./IBridge.sol"; /// @audit Out of order with the prev import️ ⬆ 47: import "../libraries/IReader4844.sol"; /// @audit Out of order with the prev import️ ⬆ 51: import {IGasRefunder} from "../libraries/IGasRefunder.sol"; /// @audit Out of order with the prev import️ ⬆ 54: import {IERC20Bridge} from "./IERC20Bridge.sol";
/// @audit Out of order with the prev import️ ⬆ 9: import "./IAssertionChain.sol";
/// @audit Out of order with the prev import️ ⬆ 10: import "../../osp/IOneStepProofEntry.sol";
/// @audit Out of order with the prev import️ ⬆ 9: import "../osp/IOneStepProofEntry.sol";
/// @audit Out of order with the prev import️ ⬆ 17: import "../bridge/IBridge.sol";
/// @audit Out of order with the prev import️ ⬆ 9: import "../bridge/ISequencerInbox.sol";
/// @audit Out of order with the prev import️ ⬆ 8: import "../bridge/IBridge.sol"; /// @audit Out of order with the prev import️ ⬆ 13: import "../challengeV2/IAssertionChain.sol";
/// @audit Out of order with the prev import️ ⬆ 10: import "../bridge/IOutbox.sol";
/// @audit Out of order with the prev import️ ⬆ 11: import "./IRollupEventInbox.sol"; /// @audit Out of order with the prev import️ ⬆ 16: import "../bridge/ISequencerInbox.sol";
/// @audit Out of order with the prev import️ ⬆ 8: import "./IRollupAdmin.sol"; /// @audit Out of order with the prev import️ ⬆ 10: import "@offchainlabs/upgrade-executor/src/IUpgradeExecutor.sol"; /// @audit Out of order with the prev import️ ⬆ 15: import {SafeERC20, IERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
/// @audit Out of order with the prev import️ ⬆ 8: import "../bridge/ISequencerInbox.sol"; /// @audit Out of order with the prev import️ ⬆ 14: import "./IRollupEventInbox.sol";
/// @audit Out of order with the prev import️ ⬆ 8: import "./IRollupAdmin.sol";
</details>/// @audit Out of order with the prev import️ ⬆ 12: import "./IRollupLogic.sol";
falseSince the bool variables are automatically set to false when created, it is redundant to initialize it with false again.
There is 1 instance:
187: bool actualIsUsingFeeToken = false;
These contracts import contracts from @openzeppelin/contracts, but they are not using the latest version. Using the latest version ensures contract security with fixes for known vulnerabilities, access to the latest feature updates, and enhanced community support and engagement.
The imported version is 4.7.3.
7: import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; 8: import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
8: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
11: import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; 12: import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
8: import "@openzeppelin/contracts/utils/Create2.sol"; 9: import "@openzeppelin/contracts/utils/Address.sol";
14: import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; 15: import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
7: import "@openzeppelin/contracts-upgradeable/utils/Create2Upgradeable.sol"; 8: import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
18: import "@openzeppelin/contracts/access/Ownable.sol"; 19: import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
13: import "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol";
7: import "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
11: import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; 12: import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; 13: import "@openzeppelin/contracts/access/Ownable.sol"; 15: import {SafeERC20, IERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
</details>7: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
These contracts import contracts from @openzeppelin/contracts-upgradeable, but they are not using the latest version. Using the latest version ensures contract security with fixes for known vulnerabilities, access to the latest feature updates, and enhanced community support and engagement.
The imported version is 4.7.3.
There are 3 instances:
14: import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
7: import "@openzeppelin/contracts-upgradeable/utils/Create2Upgradeable.sol";
7: import "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
immutable rather than constantWhile it doesn't save any gas because the compiler knows that developers often make this mistake, it's still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.
There is 1 instance:
135: bytes32 public constant UNRIVALED = keccak256(abi.encodePacked("UNRIVALED"));
Moving duplicate strings to constants can improve code maintainability and readability.
<details> <summary>There are 22 instances (click to show):</summary>32: require(startIndex < endIndex, "Start not less than end");
112: require(me.length <= MAX_LEVEL, "Merkle expansion too large"); 160: require(me.length <= MAX_LEVEL, "Merkle expansion too large"); 265: require(startSize < endSize, "Start not less than end");
15: require(x > 0, "Zero has no significant bits"); 30: require(x != 0, "Zero has no significant bits");
57: require(config.loserStakeEscrow != address(0), "INVALID_ESCROW_0"); 181: require(_validator.length > 0, "EMPTY_ARRAY"); 229: require(staker.length > 0, "EMPTY_ARRAY"); 272: require(newLoserStakerEscrow != address(0), "INVALID_ESCROW_0");
357: require(staker.isStaked, "NOT_STAKED"); 566: require(isStaked(stakerAddress), "NOT_STAKED");
154: "SI_MAX_DATA_SIZE_MISMATCH" 158: "SI_MAX_DATA_SIZE_MISMATCH" 160: require(deployParams.maxDataSize == ethInbox.maxDataSize(), "I_MAX_DATA_SIZE_MISMATCH"); 172: "SI_MAX_DATA_SIZE_MISMATCH" 176: "SI_MAX_DATA_SIZE_MISMATCH" 180: "I_MAX_DATA_SIZE_MISMATCH"
</details>63: require(!validatorWhitelistDisabled, "WHITELIST_DISABLED"); 72: require(!validatorWhitelistDisabled, "WHITELIST_DISABLED"); 175: require(isStaked(msg.sender), "NOT_STAKED"); 233: require(isStaked(stakerAddress), "NOT_STAKED");
Consider using a library instead of a contract to provide utility functions.
There are 3 instances:
13: contract AssertionStakingPoolCreator is IAssertionStakingPoolCreator {
13: contract EdgeStakingPoolCreator is IEdgeStakingPoolCreator {
11: contract RollupProxy is AdminFallbackProxy {
As the Solidity Style Guide suggests: functions should be named in mixedCase style.
There are 7 instances:
55: function HEADER_LENGTH() external view returns (uint256); 61: function DATA_AUTHENTICATED_FLAG() external view returns (bytes1); 67: function DATA_BLOB_HEADER_FLAG() external view returns (bytes1); 73: function DAS_MESSAGE_HEADER_FLAG() external view returns (bytes1); 79: function TREE_DAS_MESSAGE_HEADER_FLAG() external view returns (bytes1); 85: function BROTLI_MESSAGE_HEADER_FLAG() external view returns (bytes1); 91: function ZERO_HEAVY_MESSAGE_HEADER_FLAG() external view returns (bytes1);
immutables should use UPPER_CASE_WITH_UNDERSCORESFor immutable variable names, each word should use all capital letters, with underscores separating each word (UPPER_CASE_WITH_UNDERSCORES)
20: address public immutable stakeToken;
25: address public immutable rollup; 27: bytes32 public immutable assertionHash;
29: address public immutable challengeManager; 31: bytes32 public immutable edgeId;
116: IReader4844 public immutable reader4844; 128: uint256 public immutable maxDataSize; 129: uint256 internal immutable deployTimeChainId = block.chainid; 131: bool internal immutable hostChainIsArbitrum = ArbitrumChecker.runningOnArbitrum(); 133: bool public immutable isUsingFeeToken; 135: bool public immutable isDelayBufferable;
124: IOldRollup public immutable rollup;
112: bool internal immutable _hostChainIsArbitrum = ArbitrumChecker.runningOnArbitrum();
</details>31: uint256 internal immutable deployTimeChainId = block.chainid;
public/external functions and state variables should NOT be prefixed with underscoreIt is recommended by the Solidity Style Guide.
There is 1 instance:
102: mapping(address => Staker) public _stakerMap;
Consider adding a NatSpec comment describing why the function doesn't need a body and or the purpose it serves.
There are 2 instances:
166: function _authorizeUpgrade(address newImplementation) internal override {} 171: function _authorizeSecondaryUpgrade(address newImplementation) internal override {}
Consider adding named imports for all parent contracts.
<details> <summary>There are 25 instances (click to show):</summary>/// @audit AbsBoldStakingPool /// @audit IAssertionStakingPool 21: contract AssertionStakingPool is AbsBoldStakingPool, IAssertionStakingPool {
/// @audit IAssertionStakingPoolCreator 13: contract AssertionStakingPoolCreator is IAssertionStakingPoolCreator {
/// @audit AbsBoldStakingPool /// @audit IEdgeStakingPool 25: contract EdgeStakingPool is AbsBoldStakingPool, IEdgeStakingPool {
/// @audit IEdgeStakingPoolCreator 13: contract EdgeStakingPoolCreator is IEdgeStakingPoolCreator {
/// @audit IAbsBoldStakingPool 10: interface IAssertionStakingPool is IAbsBoldStakingPool {
/// @audit IAbsBoldStakingPool 10: interface IEdgeStakingPool is IAbsBoldStakingPool {
/// @audit IDelayedMessageProvider 15: interface ISequencerInbox is IDelayedMessageProvider {
/// @audit DelegateCallAware /// @audit ISequencerInbox 64: contract SequencerInbox is DelegateCallAware, GasRefundEnabled, ISequencerInbox {
/// @audit Initializable 206: contract EdgeChallengeManager is IEdgeChallengeManager, Initializable {
/// @audit Ownable 21: contract BridgeCreator is Ownable {
/// @audit IAssertionChain 15: interface IRollupCore is IAssertionChain {
/// @audit IRollupCore /// @audit IOwnable 12: interface IRollupUser is IRollupCore, IOwnable {
/// @audit RollupCore /// @audit IRollupAdmin /// @audit DoubleLogicUUPSUpgradeable 15: contract RollupAdminLogic is RollupCore, IRollupAdmin, DoubleLogicUUPSUpgradeable {
/// @audit IRollupCore /// @audit PausableUpgradeable 22: abstract contract RollupCore is IRollupCore, PausableUpgradeable {
/// @audit Ownable 17: contract RollupCreator is Ownable {
/// @audit AdminFallbackProxy 11: contract RollupProxy is AdminFallbackProxy {
</details>/// @audit RollupCore /// @audit UUPSNotUpgradeable 15: contract RollupUserLogic is RollupCore, UUPSNotUpgradeable, IRollupUser {
Putting constants on the left side of comparison statements is a best practice known as Yoda conditions. Although solidity's static typing system prevents accidental assignments within conditionals, adopting this practice can improve code readability and consistency, especially when working across multiple languages.
<details> <summary>There are 106 instances (click to show):</summary>/// @audit put `0` on the left 30: if (amount == 0) { /// @audit put `0` on the left 42: if (amount == 0) {
/// @audit put `0` on the left 117: config.threshold != 0 && /// @audit put `0` on the left 118: config.max != 0 && /// @audit put `BASIS` on the left 119: config.replenishRateInBasis <= BASIS &&
/// @audit put `IReader4844(address(0))` on the left 148: if (reader4844_ != IReader4844(address(0))) revert DataBlobsNotSupported(); /// @audit put `IReader4844(address(0))` on the left 150: if (reader4844_ == IReader4844(address(0))) revert InitParamZero("Reader4844"); /// @audit put `0` on the left 170: if (buffer.bufferBlocks != 0) { /// @audit put `IBridge(address(0))` on the left 182: if (bridge != IBridge(address(0))) revert AlreadyInit(); /// @audit put `IBridge(address(0))` on the left 183: if (bridge_ == IBridge(address(0))) revert HadZeroInit(); /// @audit put `address(0)` on the left 189: if (feeToken != address(0)) { /// @audit put `~uint256(0)` on the left 484: if (seqMessageIndex != sequenceNumber && sequenceNumber != ~uint256(0)) { /// @audit put `~uint256(0)` on the left 538: if (seqMessageIndex != sequenceNumber && sequenceNumber != ~uint256(0)) { /// @audit put `HEADER_LENGTH` on the left 651: assert(header.length == HEADER_LENGTH); /// @audit put `BROTLI_MESSAGE_HEADER_FLAG` on the left 677: headerByte == BROTLI_MESSAGE_HEADER_FLAG || /// @audit put `DAS_MESSAGE_HEADER_FLAG` on the left 678: headerByte == DAS_MESSAGE_HEADER_FLAG || /// @audit put `ZERO_HEAVY_MESSAGE_HEADER_FLAG` on the left 680: headerByte == ZERO_HEAVY_MESSAGE_HEADER_FLAG; /// @audit put `33` on the left 711: if (data[0] & DAS_MESSAGE_HEADER_FLAG != 0 && data.length >= 33) { /// @audit put `0` on the left 736: if (dataHashes.length == 0) revert MissingDataHashes(); /// @audit put `0` on the left 851: if (buffer.bufferBlocks == 0 || buffer.bufferBlocks > bufferConfig_.max) { /// @audit put `64 * 1024` on the left 906: if (keysetBytes.length >= 64 * 1024) revert KeysetTooLarge(); /// @audit put `0` on the left 959: if (ksInfo.creationBlock == 0) revert NoSuchKeyset(ksHash);
/// @audit put `0` on the left 325: if (_challengePeriodBlocks == 0) { /// @audit put `address(0)` on the left 331: if (_excessStakeReceiver == address(0)) { /// @audit put `0` on the left 350: if (_numBigStepLevel == 0) { /// @audit put `0` on the left 387: if (args.proof.length == 0) { /// @audit put `0` on the left 429: if (address(st) != address(0) && sa != 0) { /// @audit put `0` on the left 458: bool lowerChildAlreadyExists = lowerChildAdded.edgeId == 0; /// @audit put `0` on the left 580: if (address(st) != address(0) && sa != 0) {
/// @audit put `0` on the left 76: if (originId == 0) { /// @audit put `0` on the left 82: if (startHistoryRoot == 0) { /// @audit put `0` on the left 85: if (endHistoryRoot == 0) { /// @audit put `address(0)` on the left 103: if (staker == address(0)) { /// @audit put `0` on the left 106: if (claimId == 0) { /// @audit put `0` on the left 224: return edge.createdAtBlock != 0; /// @audit put `0` on the left 231: if (len == 0) { /// @audit put `0` on the left 240: if (edge.lowerChildId != 0 || edge.upperChildId != 0) { /// @audit put `0` on the left 240: if (edge.lowerChildId != 0 || edge.upperChildId != 0) { /// @audit put `0` on the left 259: return edge.claimId != 0 && edge.staker != address(0); /// @audit put `address(0)` on the left 259: return edge.claimId != 0 && edge.staker != address(0); /// @audit put `true` on the left 271: if (edge.refunded == true) { /// @audit put `0` on the left 280: if (level == 0) {
/// @audit put `0` on the left 182: if (firstRival == 0) { /// @audit put `UNRIVALED` on the left 184: } else if (firstRival == UNRIVALED) { /// @audit put `0` on the left 198: firstRival != 0, /// @audit put `0` on the left 199: edge.claimId != 0 /// @audit put `0` on the left 229: if (ard.assertionHash == 0) { /// @audit put `0` on the left 248: if (args.proof.length == 0) { /// @audit put `0` on the left 294: if (args.proof.length == 0) { /// @audit put `0` on the left 329: if (x == 0) { /// @audit put `0` on the left 378: if (args.prefixProof.length == 0) { /// @audit put `0` on the left 472: if (firstRival == 0) { /// @audit put `UNRIVALED` on the left 477: return firstRival != UNRIVALED; /// @audit put `bytes32(0)` on the left 490: if (store.edges[edgeId].lowerChildId != bytes32(0)) { /// @audit put `0` on the left 545: if (firstRival == 0) { /// @audit put `UNRIVALED` on the left 551: if (firstRival == UNRIVALED) { /// @audit put `bytes32(0)` on the left 665: if (confirmedRivalId != bytes32(0)) {
/// @audit put `MAX_LEVEL` on the left 112: require(me.length <= MAX_LEVEL, "Merkle expansion too large"); /// @audit put `0` on the left 117: if (accum == 0) { /// @audit put `0` on the left 118: if (val != 0) { /// @audit put `0` on the left 129: } else if (val != 0) { /// @audit put `0` on the left 159: require(subtreeRoot != 0, "Cannot append empty subtree"); /// @audit put `MAX_LEVEL` on the left 160: require(me.length <= MAX_LEVEL, "Merkle expansion too large"); /// @audit put `0` on the left 162: if (me.length == 0) { /// @audit put `MAX_LEVEL` on the left 183: require(next.length <= MAX_LEVEL, "Append creates oversize tree"); /// @audit put `0` on the left 195: require(me[i] == 0, "Append above least significant bit"); /// @audit put `0` on the left 198: if (accumHash == 0) { /// @audit put `0` on the left 203: if (me[i] == 0) { /// @audit put `0` on the left 222: if (accumHash != 0) { /// @audit put `0` on the left 228: require(next[next.length - 1] != 0, "Last entry zero"); /// @audit put `0` on the left 275: if (y != 0) { /// @audit put `0` on the left 282: if (z != 0) { /// @audit put `0` on the left 295: if (me[i] != 0) {
/// @audit put `0` on the left 30: require(x != 0, "Zero has no significant bits"); /// @audit put `0x100000000000000000000000000000000` on the left 33: if (x >= 0x100000000000000000000000000000000) { /// @audit put `0x10000000000000000` on the left 38: if (x >= 0x10000000000000000) { /// @audit put `0x100000000` on the left 43: if (x >= 0x100000000) { /// @audit put `0x10000` on the left 48: if (x >= 0x10000) { /// @audit put `0x100` on the left 53: if (x >= 0x100) { /// @audit put `0x10` on the left 58: if (x >= 0x10) { /// @audit put `0x4` on the left 63: if (x >= 0x4) { /// @audit put `0x2` on the left 68: if (x >= 0x2) msb += 1;
/// @audit put `0` on the left 86: if (self.firstChildBlock == 0) { /// @audit put `0` on the left 88: } else if (self.secondChildBlock == 0) {
/// @audit put `0` on the left 114: require(inboxMaxCount != 0, "Hash not yet set"); /// @audit put `0` on the left 353: if (staker.isStaked && staker.currentChallenge == 0) { /// @audit put `0` on the left 526: if (validators.length != 0) {
/// @audit put `0` on the left 107: bool isDelayBufferable = bufferConfig.threshold != 0; /// @audit put `address(0)` on the left 112: nativeToken == address(0) ? ethBasedTemplates : erc20BasedTemplates, /// @audit put `address(0)` on the left 117: if (nativeToken == address(0)) {
/// @audit put `address(0)` on the left 57: require(config.loserStakeEscrow != address(0), "INVALID_ESCROW_0"); /// @audit put `address(0)` on the left 272: require(newLoserStakerEscrow != address(0), "INVALID_ESCROW_0");
/// @audit put `bytes32(0)` on the left 127: require(assertionHash != bytes32(0), "ASSERTION_ID_CANNOT_BE_ZERO"); /// @audit put `0` on the left 427: require(afterStateCmpMaxInbox <= 0, "INBOX_TOO_FAR"); /// @audit put `0` on the left 466: require(afterInboxPosition != 0, "EMPTY_INBOX_COUNT"); /// @audit put `bytes32(0)` on the left 478: newAssertionHash == expectedAssertionHash || expectedAssertionHash == bytes32(0), /// @audit put `0` on the left 489: prevAssertion.firstChildBlock == 0, // assumes block 0 is impossible
/// @audit put `address(0)` on the left 228: if (deployParams.batchPosterManager != address(0)) { /// @audit put `0` on the left 233: if (deployParams.validators.length != 0) { /// @audit put `address(0)` on the left 292: if (_nativeToken == address(0)) {
</details>/// @audit put `address(0)` on the left 28: require(_stakeToken != address(0), "NEED_STAKE_TOKEN"); /// @audit put `0` on the left 53: if (latestConfirmedAssertion.createdAtBlock == 0) return false; /// @audit put `0` on the left 120: require(winningEdge.confirmedAtBlock != 0, "ZERO_CONFIRMED_AT_BLOCK"); /// @audit put `bytes32(0)` on the left 170: expectedAssertionHash == bytes32(0) /// @audit put `bytes32(0)` on the left 278: require(expectedAssertionHash != bytes32(0), "EXPECTED_ASSERTION_HASH"); /// @audit put `address(0)` on the left 337: require(withdrawalAddress != address(0), "EMPTY_WITHDRAWAL_ADDRESS");
else-block not requiredOne level of nesting can be removed by not having an else block when the if-block always jumps at the end. For example:
if (condition) { body1... return x; } else { body2... }
can be changed to:
<details> <summary>There are 12 instances (click to show):</summary>if (condition) { body1... return x; } body2...
16: if (Address.isContract(pool)) { 17: return pool; 18: } else {
279: if (_chainIdChanged()) { 280: return (1, 1, 1, 1); 281: } else {
592: if (eType == EdgeType.Block) { 593: return LAYERZERO_BLOCKEDGE_HEIGHT; 594: } else if (eType == EdgeType.BigStep) { 594: } else if (eType == EdgeType.BigStep) { 595: return LAYERZERO_BIGSTEPEDGE_HEIGHT; 596: } else if (eType == EdgeType.SmallStep) { 596: } else if (eType == EdgeType.SmallStep) { 597: return LAYERZERO_SMALLSTEPEDGE_HEIGHT; 598: } else {
280: if (level == 0) { 281: return EdgeType.Block; 282: } else if (level <= numBigStepLevels) { 282: } else if (level <= numBigStepLevels) { 283: return EdgeType.BigStep; 284: } else if (level == numBigStepLevels + 1) { 284: } else if (level == numBigStepLevels + 1) { 285: return EdgeType.SmallStep; 286: } else {
220: if (ChallengeEdgeLib.levelToType(args.level, numBigStepLevel) == EdgeType.Block) { 221: // origin id is the assertion which is the root of challenge 222: // all rivals and their children share the same origin id - it is a link to the information 223: // they agree on 224: bytes32 originId = ard.predecessorId; 225: 226: // Sanity check: The assertion reference data should be related to the claim 227: // Of course the caller can provide whatever args they wish, so this is really just a helpful 228: // check to avoid mistakes 229: if (ard.assertionHash == 0) { 230: revert AssertionHashEmpty(); 231: } 232: if (ard.assertionHash != args.claimId) { 233: revert AssertionHashMismatch(ard.assertionHash, args.claimId); 234: } 235: 236: // if the assertion is already confirmed or rejected then it cant be referenced as a claim 237: if (!ard.isPending) { 238: revert AssertionNotPending(); 239: } 240: 241: // if the claim doesnt have a sibling then it is undisputed, there's no need 242: // to open challenge edges for it 243: if (!ard.hasSibling) { 244: revert AssertionNoSibling(); 245: } 246: 247: // parse the inclusion proof for later use 248: if (args.proof.length == 0) { 249: revert EmptyEdgeSpecificProof(); 250: } 251: (bytes32[] memory inclusionProof,,) = 252: abi.decode(args.proof, (bytes32[], AssertionStateData, AssertionStateData)); 253: 254: // check the start and end execution states exist, the block hash entry should be non zero 255: if (ard.startState.machineStatus == MachineStatus.RUNNING) { 256: revert EmptyStartMachineStatus(); 257: } 258: if (ard.endState.machineStatus == MachineStatus.RUNNING) { 259: revert EmptyEndMachineStatus(); 260: } 261: 262: // Create machine hashes out of the proven state 263: bytes32 startStateHash = oneStepProofEntry.getMachineHash(ard.startState.toExecutionState()); 264: bytes32 endStateHash = oneStepProofEntry.getMachineHash(ard.endState.toExecutionState()); 265: 266: return (ProofData(startStateHash, endStateHash, inclusionProof), originId); 267: } else { 551: if (firstRival == UNRIVALED) { 552: return block.number - store.edges[edgeId].createdAtBlock; 553: } else { 562: if (firstRivalCreatedAtBlock > edgeCreatedAtBlock) { 563: // if this edge was created before the first rival then we return the difference 564: // in createdAtBlock number 565: return firstRivalCreatedAtBlock - edgeCreatedAtBlock; 566: } else {
</details>146: if (_hostChainIsArbitrum) { 147: uint256 blockNum = _assertionCreatedAtArbSysBlock[assertionHash]; 148: require(blockNum > 0, "NO_ASSERTION"); 149: return blockNum; 150: } else {
Use a scientific notation rather than decimal literals (e.g. 1e6 instead of 1000000), for better code readability.
There is 1 instance:
/// @audit 10000 -> 1e4 17: uint256 public constant BASIS = 10000;
TODOs left in the codeTODOs may signal that a feature is missing or not ready for audit, consider resolving the issue and removing the TODO comment
There is 1 instance:
63: /// @notice TODO
Consider breaking down these blocks into more manageable units, by splitting things into utility functions, by reducing nesting, and by using early returns.
<details> <summary>There is 1 instance (click to show):</summary></details>305: function initialize( 306: IAssertionChain _assertionChain, 307: uint64 _challengePeriodBlocks, 308: IOneStepProofEntry _oneStepProofEntry, 309: uint256 layerZeroBlockEdgeHeight, 310: uint256 layerZeroBigStepEdgeHeight, 311: uint256 layerZeroSmallStepEdgeHeight, 312: IERC20 _stakeToken, 313: address _excessStakeReceiver, 314: uint8 _numBigStepLevel, 315: uint256[] calldata _stakeAmounts 316: ) public initializer { 317: if (address(_assertionChain) == address(0)) { 318: revert EmptyAssertionChain(); 319: } 320: assertionChain = _assertionChain; 321: if (address(_oneStepProofEntry) == address(0)) { 322: revert EmptyOneStepProofEntry(); 323: } 324: oneStepProofEntry = _oneStepProofEntry; 325: if (_challengePeriodBlocks == 0) { 326: revert EmptyChallengePeriod(); 327: } 328: challengePeriodBlocks = _challengePeriodBlocks; 329: ...... OMITTED ...... 340: if (!EdgeChallengeManagerLib.isPowerOfTwo(layerZeroBigStepEdgeHeight)) { 341: revert NotPowerOfTwo(layerZeroBigStepEdgeHeight); 342: } 343: LAYERZERO_BIGSTEPEDGE_HEIGHT = layerZeroBigStepEdgeHeight; 344: if (!EdgeChallengeManagerLib.isPowerOfTwo(layerZeroSmallStepEdgeHeight)) { 345: revert NotPowerOfTwo(layerZeroSmallStepEdgeHeight); 346: } 347: LAYERZERO_SMALLSTEPEDGE_HEIGHT = layerZeroSmallStepEdgeHeight; 348: 349: // ensure that there is at least on of each type of level 350: if (_numBigStepLevel == 0) { 351: revert ZeroBigStepLevels(); 352: } 353: // ensure there's also space for the block level and the small step level 354: // in total level parameters 355: if (_numBigStepLevel > 253) { 356: revert BigStepLevelsTooMany(_numBigStepLevel); 357: } 358: NUM_BIGSTEP_LEVEL = _numBigStepLevel; 359: 360: if (_numBigStepLevel + 2 != _stakeAmounts.length) { 361: revert StakeAmountsMismatch(_stakeAmounts.length, _numBigStepLevel + 2); 362: } 363: stakeAmounts = _stakeAmounts; 364: }
All typos should be corrected.
<details> <summary>There are 18 instances (click to show):</summary>/// @audit proccessed 608: // buffer update depends on new delayed messages. if none are read, no buffer update is proccessed
/// @audit exlusive 23: /// @dev End index exlusive so slice(arr, 0, 5) gets the first 5 elements of arr /// @audit exlusive 26: /// @param endIndex The end index of the slice in the original array - exlusive
/// @audit existance 274: // hasLengthOneRival checks existance, so we can use getNoCheck /// @audit preceeding 793: // To do this we sum the machine steps of the edges in each of the preceeding levels.
/// @audit leve 156: // we use number representations of the levels elsewhere, so we need to ensure we're appending a leve /// @audit numbe 176: // if by appending the sub tree we increase the numbe of most sig bits of the size, that means /// @audit storeage 221: // increased the storeage above
/// @audit signficant 13: /// @param x Cannot be zero, since zero that has no signficant bits /// @audit sigificant 28: /// @param x Cannot be zero, since zero has no sigificant bits
/// @audit adddress 35: /// @param addr The adddress in question
/// @audit UNEXPCTED 517: require(address(rollup) == expectedRollupAddress, "UNEXPCTED_ROLLUP_ADDR");
/// @audit lastest 570: bytes32 lastestAssertion = latestStakedAssertion(stakerAddress); /// @audit lastest 571: bool isLatestConfirmed = lastestAssertion == latestConfirmed(); /// @audit lastest 572: bool haveChild = getAssertionStorage(lastestAssertion).firstChildBlock > 0;
/// @audit emited 53: // All these should be emited in AssertionCreated event
</details>/// @audit chlid 220: * @notice Refund a staker that is currently staked on an assertion that either has a chlid assertion or is the latest confirmed assertion. /// @audit creditted 238: * @notice Reduce the amount staked for the sender (difference between initial amount staked and target is creditted back to the sender).
The functions below take in an unbounded array, and make function calls for entries in the array. While the function will revert if it eventually runs out of gas, it may be a nicer user experience to require() that the length of the array is below some reasonable maximum, so that the user doesn't have to use up a full transaction's gas only to see that the transaction reverts.
There are 4 instances:
492: for (uint256 i = 0; i < edgeIds.length; i++) {
528: for (uint256 i = 0; i < validators.length; i++) {
184: for (uint256 i = 0; i < _validator.length; i++) { 230: for (uint256 i = 0; i < staker.length; i++) {
Unnecessary castings can be removed.
<details> <summary>There are 11 instances (click to show):</summary>/// @audit address(challengeManager) 46: IERC20(stakeToken).safeIncreaseAllowance(address(challengeManager), requiredStake);
/// @audit IOwnable(rollup) 209: if (msg.sender != IOwnable(rollup).owner()) /// @audit IOwnable(rollup) 210: revert NotOwner(msg.sender, IOwnable(rollup).owner());
/// @audit address(IMPL_CHALLENGE_MANAGER) 475: address(IMPL_CHALLENGE_MANAGER),
/// @audit IBridge(frame.bridge) 122: frame.sequencerInbox.initialize(IBridge(frame.bridge), maxTimeVariation, bufferConfig);
/// @audit address(_inbox) 139: bridge.setDelayedInbox(address(_inbox), _enabled);
/// @audit address(upgradeExecutor) 204: proxyAdmin.transferOwnership(address(upgradeExecutor)); /// @audit address(upgradeExecutor) 241: IRollupAdmin(address(rollup)).setOwner(address(upgradeExecutor)); /// @audit address(upgradeExecutor) 261: address(upgradeExecutor), /// @audit address(validatorWalletCreator) 262: address(validatorWalletCreator)
</details>/// @audit address(connectedContracts.rollupAdminLogic) 21: address(connectedContracts.rollupAdminLogic),
The following state variables are defined but not used. It is recommended to check the code for logical omissions that cause them not to be used. If it's determined that they are not needed anywhere, it's best to remove them from the codebase to improve code clarity and minimize confusion.
There is 1 instance:
99: ISequencerInbox.MaxTimeVariation private __LEGACY_MAX_TIME_VARIATION;
Function parameters that are defined but not used may be logical errors and need to be checked to see if any logic is missing. If the parameter is not really needed, it should be removed, otherwise it will repeatedly cause confusion and make code maintenance difficult. If the parameter cannot be removed directly (for example, if it is an override function), its name should be removed and some comment can be appended.
There are 2 instances:
/// @audit newImplementation 166: function _authorizeUpgrade(address newImplementation) internal override {} /// @audit newImplementation 171: function _authorizeSecondaryUpgrade(address newImplementation) internal override {}
Declaring named returns, but not using them, is confusing to the reader. Consider either completely removing them (by declaring just the type without a name), or remove the return statement and do a variable assignment. This would improve the readability of the code, and it may also help reduce regressions during future code refactors.
There are 2 instances:
/// @audit eType 279: function levelToType(uint8 level, uint8 numBigStepLevels) internal pure returns (EdgeType eType) {
/// @audit msb 14: function leastSignificantBit(uint256 x) internal pure returns (uint256 msb) {
falseThe delete keyword more closely matches the semantics of what is being done, and draws more attention to the changing of state, which may lead to a more thorough audit of its associated logic.
There is 1 instance:
927: dasKeySetInfo[ksHash].isValidKeyset = false;
delete rather than assigning zero to clear valuesThe delete keyword more closely matches the semantics of what is being done, and draws more attention to the changing of state, which may lead to a more thorough audit of its associated logic.
There are 3 instances:
207: accumHash = 0; 212: next[i] = 0;
333: _withdrawableFunds[account] = 0;
Upgrading to the latest solidity version (0.8.19 for L2s) can optimize gas usage, take advantage of new features and improve overall contract efficiency. Where possible, based on compatibility requirements, it is recommended to use newer/latest solidity version to take advantage of the latest optimizations and features.
<details> <summary>There are 24 instances (click to show):</summary>5: pragma solidity ^0.8.0;
6: pragma solidity ^0.8.0;
6: pragma solidity ^0.8.0;
5: pragma solidity ^0.8.0;
6: pragma solidity ^0.8.0;
6: pragma solidity ^0.8.0;
5: pragma solidity ^0.8.0;
5: pragma solidity ^0.8.0;
5: pragma solidity ^0.8.17;
5: pragma solidity ^0.8.17;
5: pragma solidity ^0.8.17;
5: pragma solidity ^0.8.17;
5: pragma solidity ^0.8.17;
5: pragma solidity ^0.8.17;
5: pragma solidity ^0.8.0;
5: pragma solidity ^0.8.0;
5: pragma solidity ^0.8.0;
5: pragma solidity ^0.8.0;
5: pragma solidity ^0.8.0;
5: pragma solidity ^0.8.0;
5: pragma solidity ^0.8.0;
5: pragma solidity ^0.8.0;
5: pragma solidity ^0.8.0;
</details>5: pragma solidity ^0.8.0;
Consider using SafeTransferLib.safeTransferETH or Address.sendValue for clearer semantic meaning instead of using a low level call.
There is 1 instance:
304: (bool sent, ) = msg.sender.call{value: address(this).balance}("");
The use of the underscore at the end of the variable name is unusual, consider refactoring it.
<details> <summary>There are 27 instances (click to show):</summary>/// @audit maxTimeVariation_ 247: function setMaxTimeVariation(MaxTimeVariation memory maxTimeVariation_) external; /// @audit isBatchPoster_ 254: function setIsBatchPoster(address addr, bool isBatchPoster_) external; /// @audit isSequencer_ 274: function setIsSequencer(address addr, bool isSequencer_) external; /// @audit bridge_ 288: IBridge bridge_, /// @audit maxTimeVariation_ 289: MaxTimeVariation calldata maxTimeVariation_, /// @audit bufferConfig_ 290: BufferConfig calldata bufferConfig_
/// @audit reader4844_ 142: IReader4844 reader4844_, /// @audit bufferConfig_ 161: function postUpgradeInit(BufferConfig memory bufferConfig_) /// @audit bridge_ 178: IBridge bridge_, /// @audit maxTimeVariation_ 179: ISequencerInbox.MaxTimeVariation calldata maxTimeVariation_, /// @audit bufferConfig_ 180: BufferConfig memory bufferConfig_ /// @audit delayBlocks_ 219: uint64 delayBlocks_, /// @audit futureBlocks_ 220: uint64 futureBlocks_, /// @audit delaySeconds_ 221: uint64 delaySeconds_, /// @audit futureSeconds_ 222: uint64 futureSeconds_ /// @audit delayBlocks_ 255: uint64 delayBlocks_, /// @audit futureBlocks_ 256: uint64 futureBlocks_, /// @audit delaySeconds_ 257: uint64 delaySeconds_, /// @audit futureSeconds_ 258: uint64 futureSeconds_ /// @audit delayBlocks_ 306: uint256 delayBlocks_ = delayBlocks; /// @audit bufferConfig_ 847: function _setBufferConfig(BufferConfig memory bufferConfig_) internal { /// @audit maxTimeVariation_ 867: function _setMaxTimeVariation(ISequencerInbox.MaxTimeVariation memory maxTimeVariation_) /// @audit maxTimeVariation_ 885: function setMaxTimeVariation(ISequencerInbox.MaxTimeVariation memory maxTimeVariation_) /// @audit isBatchPoster_ 894: function setIsBatchPoster(address addr, bool isBatchPoster_) /// @audit isSequencer_ 933: function setIsSequencer(address addr, bool isSequencer_) /// @audit bufferConfig_ 947: function setBufferConfig(BufferConfig memory bufferConfig_) external onlyRollupOwner {
</details>/// @audit bufferConfig_ 84: function postUpgradeInit(BufferConfig memory bufferConfig_) external;
If a function has too many parameters, replacing them with a struct can improve code readability and maintainability, increase reusability, and reduce the likelihood of errors when passing the parameters.
<details> <summary>There are 9 instances (click to show):</summary>139: function forceInclusion( 140: uint256 _totalDelayedMessagesRead, 141: uint8 kind, 142: uint64[2] calldata l1BlockAndTime, 143: uint256 baseFeeL1, 144: address sender, 145: bytes32 messageDataHash 146: ) external; 179: function addSequencerL2BatchFromOrigin( 180: uint256 sequenceNumber, 181: bytes calldata data, 182: uint256 afterDelayedMessagesRead, 183: IGasRefunder gasRefunder, 184: uint256 prevMessageCount, 185: uint256 newMessageCount 186: ) external; 188: function addSequencerL2Batch( 189: uint256 sequenceNumber, 190: bytes calldata data, 191: uint256 afterDelayedMessagesRead, 192: IGasRefunder gasRefunder, 193: uint256 prevMessageCount, 194: uint256 newMessageCount 195: ) external; 197: function addSequencerL2BatchFromBlobs( 198: uint256 sequenceNumber, 199: uint256 afterDelayedMessagesRead, 200: IGasRefunder gasRefunder, 201: uint256 prevMessageCount, 202: uint256 newMessageCount 203: ) external; 207: function addSequencerL2BatchFromBlobsDelayProof( 208: uint256 sequenceNumber, 209: uint256 afterDelayedMessagesRead, 210: IGasRefunder gasRefunder, 211: uint256 prevMessageCount, 212: uint256 newMessageCount, 213: DelayProof calldata delayProof 214: ) external; 219: function addSequencerL2BatchFromOriginDelayProof( 220: uint256 sequenceNumber, 221: bytes calldata data, 222: uint256 afterDelayedMessagesRead, 223: IGasRefunder gasRefunder, 224: uint256 prevMessageCount, 225: uint256 newMessageCount, 226: DelayProof calldata delayProof 227: ) external; 231: function addSequencerL2BatchDelayProof( 232: uint256 sequenceNumber, 233: bytes calldata data, 234: uint256 afterDelayedMessagesRead, 235: IGasRefunder gasRefunder, 236: uint256 prevMessageCount, 237: uint256 newMessageCount, 238: DelayProof calldata delayProof 239: ) external;
</details>287: function forceInclusion( 288: uint256 _totalDelayedMessagesRead, 289: uint8 kind, 290: uint64[2] calldata l1BlockAndTime, 291: uint256 baseFeeL1, 292: address sender, 293: bytes32 messageDataHash 294: ) external { 366: function addSequencerL2BatchFromOrigin( 367: uint256 sequenceNumber, 368: bytes calldata data, 369: uint256 afterDelayedMessagesRead, 370: IGasRefunder gasRefunder, 371: uint256 prevMessageCount, 372: uint256 newMessageCount 373: ) external refundsGas(gasRefunder, IReader4844(address(0))) {
If a function returns too many variables, replacing them with a struct can improve code readability, maintainability and reusability.
<details> <summary>There are 7 instances (click to show):</summary>114: function maxTimeVariation() 115: external 116: view 117: returns ( 118: uint256 delayBlocks, 119: uint256 futureBlocks, 120: uint256 delaySeconds, 121: uint256 futureSeconds 122: ); 124: function dasKeySetInfo(bytes32) external view returns (bool, uint64);
</details>244: function maxTimeVariation() 245: external 246: view 247: returns ( 248: uint256, 249: uint256, 250: uint256, 251: uint256 252: ) 269: function maxTimeVariationInternal() 270: internal 271: view 272: returns ( 273: uint64, 274: uint64, 275: uint64, 276: uint64 277: ) 637: function packHeader(uint256 afterDelayedMessagesRead) 638: internal 639: view 640: returns (bytes memory, IBridge.TimeBounds memory) 659: function formEmptyDataHash(uint256 afterDelayedMessagesRead) 660: internal 661: view 662: returns (bytes32, IBridge.TimeBounds memory) 688: function formCallDataHash(bytes calldata data, uint256 afterDelayedMessagesRead) 689: internal 690: view 691: returns (bytes32, IBridge.TimeBounds memory)
Consider adding some comments on non-public contract variables to explain what they are supposed to do. This will help for future code reviews.
<details> <summary>There are 11 instances (click to show):</summary>120: uint64 internal futureBlocks; 121: uint64 internal delaySeconds; 122: uint64 internal futureSeconds; 129: uint256 internal immutable deployTimeChainId = block.chainid;
99: mapping(bytes32 => bytes) internal preImages; 167: uint256[] _array;
98: bytes32 private _latestConfirmed; 99: mapping(bytes32 => AssertionNode) private _assertions; 101: address[] private _stakerList; 104: mapping(address => uint256) private _withdrawableFunds;
</details>31: uint256 internal immutable deployTimeChainId = block.chainid;
The initial states set in a constructor are important and should be recorded in the event.
There are 3 instances:
170: _array = __array;
49: ethBasedTemplates = _ethBasedTemplates; 50: erc20BasedTemplates = _erc20BasedTemplates;
Passing empty bytes to a function can cause unexpected behavior, such as certain operations failing, producing incorrect results, or wasting gas. It is recommended to check that all byte parameters are not empty.
<details> <summary>There are 5 instances (click to show):</summary>/// @audit data 366: function addSequencerL2BatchFromOrigin( 367: uint256 sequenceNumber, 368: bytes calldata data, 369: uint256 afterDelayedMessagesRead, 370: IGasRefunder gasRefunder, 371: uint256 prevMessageCount, 372: uint256 newMessageCount 373: ) external refundsGas(gasRefunder, IReader4844(address(0))) { /// @audit data 430: function addSequencerL2BatchFromOriginDelayProof( 431: uint256 sequenceNumber, 432: bytes calldata data, 433: uint256 afterDelayedMessagesRead, 434: IGasRefunder gasRefunder, 435: uint256 prevMessageCount, 436: uint256 newMessageCount, 437: DelayProof calldata delayProof 438: ) external refundsGas(gasRefunder, IReader4844(address(0))) { /// @audit data 560: function addSequencerL2Batch( 561: uint256 sequenceNumber, 562: bytes calldata data, 563: uint256 afterDelayedMessagesRead, 564: IGasRefunder gasRefunder, 565: uint256 prevMessageCount, 566: uint256 newMessageCount 567: ) external override refundsGas(gasRefunder, IReader4844(address(0))) { /// @audit data 582: function addSequencerL2BatchDelayProof( 583: uint256 sequenceNumber, 584: bytes calldata data, 585: uint256 afterDelayedMessagesRead, 586: IGasRefunder gasRefunder, 587: uint256 prevMessageCount, 588: uint256 newMessageCount, 589: DelayProof calldata delayProof 590: ) external refundsGas(gasRefunder, IReader4844(address(0))) {
</details>/// @audit prefixProof 451: function bisectEdge(bytes32 edgeId, bytes32 bisectionHistoryRoot, bytes calldata prefixProof) 452: external 453: returns (bytes32, bytes32) 454: {
In Solidity, while function overriding allows for functions with the same name to coexist, it is advisable to avoid this practice to enhance code readability and maintainability. Having multiple functions with the same name, even with different parameters or in inherited contracts, can cause confusion and increase the likelihood of errors during development, testing, and debugging. Using distinct and descriptive function names not only clarifies the purpose and behavior of each function, but also helps prevent unintended function calls or incorrect overriding. By adopting a clear and consistent naming convention, developers can create more comprehensible and maintainable smart contracts.
<details> <summary>There are 7 instances (click to show):</summary>/// @audit Different function with same name found on line 41 57: function withdrawFromPool() external {
/// @audit Different function with same name found on line 24 27: function withdrawFromPool() external;
/// @audit Different function with same name found on line 171 179: function addSequencerL2BatchFromOrigin(
/// @audit Different function with same name found on line 356 366: function addSequencerL2BatchFromOrigin(
/// @audit Different function with same name found on line 38 44: function newStakeOnNewAssertion(
/// @audit Different function with same name found on line 23 37: function assertionHash(
</details>/// @audit Different function with same name found on line 316 331: function newStakeOnNewAssertion(
Well-organized data structures make code reviews easier, which may lead to fewer bugs. Consider combining related mappings into mappings to structs, so it's clear what data is related.
There are 7 instances:
95: mapping(address => bool) public isBatchPoster; 115: mapping(address => bool) public isSequencer;
96: mapping(address => bool) public isValidator; 99: mapping(bytes32 => AssertionNode) private _assertions; 102: mapping(address => Staker) public _stakerMap; 104: mapping(address => uint256) private _withdrawableFunds; 114: mapping(bytes32 => uint256) internal _assertionCreatedAtArbSysBlock;
Instead of caching immutable variables, using them directly can make code more consistent and less prone to errors.
There are 5 instances:
415: TransparentUpgradeableProxy bridge = TransparentUpgradeableProxy(payable(BRIDGE)); 421: TransparentUpgradeableProxy inbox = TransparentUpgradeableProxy(payable(INBOX)); 424: TransparentUpgradeableProxy rollupEventInbox = TransparentUpgradeableProxy(payable(REI)); 428: TransparentUpgradeableProxy outbox = TransparentUpgradeableProxy(payable(OUTBOX)); 434: TransparentUpgradeableProxy sequencerInbox = TransparentUpgradeableProxy(payable(SEQ_INBOX));
Events should be emitted when critical changes are made to the contracts.
<details> <summary>There are 17 instances (click to show):</summary>68: function update(BufferData storage self, uint64 blockNumber) internal { 69: self.bufferBlocks = calcPendingBuffer(self, blockNumber); 70: 71: // store a new starting reference point 72: // any buffer updates will be applied retroactively in the next batch post 73: self.prevBlockNumber = blockNumber; 74: self.prevSequencedBlockNumber = uint64(block.number); 75: }
208: function updateRollupAddress() external { 209: if (msg.sender != IOwnable(rollup).owner()) 210: revert NotOwner(msg.sender, IOwnable(rollup).owner()); 211: IOwnable newRollup = bridge.rollup(); 212: if (rollup == newRollup) revert RollupNotChanged(); 213: rollup = newRollup; 214: } 236: function removeDelayAfterFork() external { 237: if (!_chainIdChanged()) revert NotForked(); 238: delayBlocks = 1; 239: futureBlocks = 1; 240: delaySeconds = 1; 241: futureSeconds = 1; 242: } 791: function addSequencerL2BatchImpl( 792: bytes32 dataHash, 793: uint256 afterDelayedMessagesRead, 794: uint256 calldataLengthPosted, 795: uint256 prevMessageCount, 796: uint256 newMessageCount 797: ) 798: internal 799: returns ( 800: uint256 seqMessageIndex, 801: bytes32 beforeAcc, 802: bytes32 delayedAcc, 803: bytes32 acc 804: ) 847: function _setBufferConfig(BufferConfig memory bufferConfig_) internal { 848: if (!isDelayBufferable) revert NotDelayBufferable(); 849: if (!DelayBuffer.isValidBufferConfig(bufferConfig_)) revert BadBufferConfig(); 850: 851: if (buffer.bufferBlocks == 0 || buffer.bufferBlocks > bufferConfig_.max) { 852: buffer.bufferBlocks = bufferConfig_.max; 853: } 854: if (buffer.bufferBlocks < bufferConfig_.threshold) { 855: buffer.bufferBlocks = bufferConfig_.threshold; 856: } 857: buffer.max = bufferConfig_.max; 858: buffer.threshold = bufferConfig_.threshold; 859: buffer.replenishRateInBasis = bufferConfig_.replenishRateInBasis; 860: 861: // if all delayed messages are read, the buffer is considered synced 862: if (bridge.delayedMessageCount() == totalDelayedMessagesRead) { 863: buffer.update(uint64(block.number)); 864: } 865: } 867: function _setMaxTimeVariation(ISequencerInbox.MaxTimeVariation memory maxTimeVariation_) 868: internal 869: { 870: if ( 871: maxTimeVariation_.delayBlocks > type(uint64).max || 872: maxTimeVariation_.futureBlocks > type(uint64).max || 873: maxTimeVariation_.delaySeconds > type(uint64).max || 874: maxTimeVariation_.futureSeconds > type(uint64).max 875: ) { 876: revert BadMaxTimeVariation(); 877: } 878: delayBlocks = uint64(maxTimeVariation_.delayBlocks); 879: futureBlocks = uint64(maxTimeVariation_.futureBlocks); 880: delaySeconds = uint64(maxTimeVariation_.delaySeconds); 881: futureSeconds = uint64(maxTimeVariation_.futureSeconds); 882: }
239: function setChildren(ChallengeEdge storage edge, bytes32 lowerChildId, bytes32 upperChildId) internal { 240: if (edge.lowerChildId != 0 || edge.upperChildId != 0) { 241: revert ChildrenAlreadySet(ChallengeEdgeLib.id(edge), edge.lowerChildId, edge.upperChildId); 242: } 243: edge.lowerChildId = lowerChildId; 244: edge.upperChildId = upperChildId; 245: } 249: function setConfirmed(ChallengeEdge storage edge) internal { 250: if (edge.status != EdgeStatus.Pending) { 251: revert EdgeNotPending(ChallengeEdgeLib.id(edge), edge.status); 252: } 253: edge.status = EdgeStatus.Confirmed; 254: edge.confirmedAtBlock = uint64(block.number); 255: } 264: function setRefunded(ChallengeEdge storage edge) internal { 265: if (edge.status != EdgeStatus.Confirmed) { 266: revert EdgeNotConfirmed(ChallengeEdgeLib.id(edge), edge.status); 267: } 268: if (!isLayerZero(edge)) { 269: revert EdgeNotLayerZero(ChallengeEdgeLib.id(edge), edge.staker, edge.claimId); 270: } 271: if (edge.refunded == true) { 272: revert EdgeAlreadyRefunded(ChallengeEdgeLib.id(edge)); 273: } 274: 275: edge.refunded = true; 276: }
160: function add(EdgeStore storage store, ChallengeEdge memory edge) internal returns (EdgeAddedData memory) { 502: function updateTimerCache(EdgeStore storage store, bytes32 edgeId, uint256 newValue) 503: internal 504: returns (bool, uint256) 505: { 506: uint256 currentAccuTimer = store.edges[edgeId].totalTimeUnrivaledCache; 507: newValue = newValue > type(uint64).max ? type(uint64).max : newValue; 508: // only update when increased 509: if (newValue > currentAccuTimer) { 510: store.edges[edgeId].totalTimeUnrivaledCache = uint64(newValue); 511: return (true, newValue); 512: } 513: return (false, currentAccuTimer); 514: } 520: function updateTimerCacheByClaim( 521: EdgeStore storage store, 522: bytes32 edgeId, 523: bytes32 claimingEdgeId, 524: uint8 numBigStepLevel 525: ) internal returns (bool, uint256) { 526: // calculate the time unrivaled without inheritance 527: uint256 totalTimeUnrivaled = timeUnrivaled(store, edgeId); 528: checkClaimIdLink(store, edgeId, claimingEdgeId, numBigStepLevel); 529: totalTimeUnrivaled += store.edges[claimingEdgeId].totalTimeUnrivaledCache; 530: return updateTimerCache(store, edgeId, totalTimeUnrivaled); 531: } 662: function setConfirmedRival(EdgeStore storage store, bytes32 edgeId) internal { 663: bytes32 mutualId = store.edges[edgeId].mutualId(); 664: bytes32 confirmedRivalId = store.confirmedRivals[mutualId]; 665: if (confirmedRivalId != bytes32(0)) { 666: revert RivalEdgeConfirmed(edgeId, confirmedRivalId); 667: } 668: store.confirmedRivals[mutualId] = edgeId; 669: }
355: function deleteStaker(address stakerAddress) private { 356: Staker storage staker = _stakerMap[stakerAddress]; 357: require(staker.isStaked, "NOT_STAKED"); 358: uint64 stakerIndex = staker.index; 359: _stakerList[stakerIndex] = _stakerList[_stakerList.length - 1]; 360: _stakerMap[_stakerList[stakerIndex]].index = stakerIndex; 361: _stakerList.pop(); 362: delete _stakerMap[stakerAddress]; 363: }
267: function _deployUpgradeExecutor(address rollupOwner, ProxyAdmin proxyAdmin) 268: internal 269: returns (address) 270: { 271: IUpgradeExecutor upgradeExecutor = IUpgradeExecutor( 272: address( 273: new TransparentUpgradeableProxy( 274: address(upgradeExecutorLogic), 275: address(proxyAdmin), 276: bytes("") 277: ) 278: ) 279: ); 280: address[] memory executors = new address[](1); 281: executors[0] = rollupOwner; 282: upgradeExecutor.initialize(address(upgradeExecutor), executors); 283: 284: return address(upgradeExecutor); 285: }
</details>62: function removeWhitelistAfterFork() external { 63: require(!validatorWhitelistDisabled, "WHITELIST_DISABLED"); 64: require(_chainIdChanged(), "CHAIN_ID_NOT_CHANGED"); 65: validatorWhitelistDisabled = true; 66: } 71: function removeWhitelistAfterValidatorAfk() external { 72: require(!validatorWhitelistDisabled, "WHITELIST_DISABLED"); 73: require(_validatorIsAfk(), "VALIDATOR_NOT_AFK"); 74: validatorWhitelistDisabled = true; 75: }
Adding a way to quickly halt protocol functionality in an emergency, rather than having to pause individual contracts one-by-one, will make in-progress hack mitigation faster and much less stressful.
<details> <summary>There are 6 instances (click to show):</summary>206: contract EdgeChallengeManager is IEdgeChallengeManager, Initializable {
21: contract BridgeCreator is Ownable {
15: contract RollupAdminLogic is RollupCore, IRollupAdmin, DoubleLogicUUPSUpgradeable {
17: contract RollupCreator is Ownable {
11: contract RollupProxy is AdminFallbackProxy {
</details>15: contract RollupUserLogic is RollupCore, UUPSNotUpgradeable, IRollupUser {
Use alternative variants, e.g. allowlist/denylist instead of whitelist/blacklist.
<details> <summary>There are 63 instances (click to show):</summary>194: /// This is only tracked when the validator whitelist is enabled 372: // Check if whitelist is enabled in the Rollup 373: // We only enforce whitelist in this function as it may exhaust resources 374: bool whitelistEnabled = !assertionChain.validatorWhitelistDisabled(); 376: if (whitelistEnabled && !assertionChain.isValidator(msg.sender)) { 418: args, ard, oneStepProofEntry, expectedEndHeight, NUM_BIGSTEP_LEVEL, whitelistEnabled
27: function validatorWhitelistDisabled() external view returns (bool);
107: /// @dev Thrown when the validator whitelist is enabled and the account attempting to create a layer zero edge is not whitelisted
112: /// @notice The id of the assertion - will be used in a sanity check 226: // Sanity check: The assertion reference data should be related to the claim 361: // However it's a nice sanity check for the calling code to check that their local edge 418: bool whitelistEnabled 428: // if the validator whitelist is enabled, we can enforce that a single party cannot create two layer zero edges that rival each other 429: // if the validator whitelist is disabled, this check serves no purpose since an attacker can create new accounts 430: if (whitelistEnabled) { 471: // Sanity check: it should never be possible to create an edge without having an entry in firstRivals 544: // Sanity check: it's not possible to have a 0 first rival for an edge that exists 554: // Sanity check: it's not possible an edge does not exist for a first rival record 684: /// @dev Does some additional sanity checks to ensure that the claim id link is valid
227: // so this is just a sanity check
204: bool public immutable DISABLE_VALIDATOR_WHITELIST; 245: bool disableValidatorWhitelist; 327: DISABLE_VALIDATOR_WHITELIST = settings.disableValidatorWhitelist; 534: if (DISABLE_VALIDATOR_WHITELIST) { 535: IRollupAdmin(address(rollup)).setValidatorWhitelistDisabled(DISABLE_VALIDATOR_WHITELIST);
48: * @notice Set the addresses of the validator whitelist 51: * @param _validator addresses to set in the whitelist 52: * @param _val value to set in the whitelist for corresponding address 110: * @notice set the validatorWhitelistDisabled flag 111: * @param _validatorWhitelistDisabled new value of validatorWhitelistDisabled, i.e. true = disabled 113: function setValidatorWhitelistDisabled(bool _validatorWhitelistDisabled) external;
17: function removeWhitelistAfterFork() external; 19: function removeWhitelistAfterValidatorAfk() external;
174: * @notice Set the addresses of the validator whitelist 177: * @param _validator addresses to set in the whitelist 178: * @param _val value to set in the whitelist for corresponding address 305: * @notice set the validatorWhitelistDisabled flag 306: * @param _validatorWhitelistDisabled new value of validatorWhitelistDisabled, i.e. true = disabled 308: function setValidatorWhitelistDisabled(bool _validatorWhitelistDisabled) external { 309: validatorWhitelistDisabled = _validatorWhitelistDisabled;
108: bool public validatorWhitelistDisabled; 377: // we can do a quick sanity check here
</details>21: require(isValidator[msg.sender] || validatorWhitelistDisabled, "NOT_VALIDATOR"); 38: * @notice Number of blocks since the last confirmed assertion before the validator whitelist is removed 41: * a further 14 days before the validator whitelist is removed. 62: function removeWhitelistAfterFork() external { 63: require(!validatorWhitelistDisabled, "WHITELIST_DISABLED"); 65: validatorWhitelistDisabled = true; 69: * @notice Remove the whitelist after the validator has been inactive for too long 71: function removeWhitelistAfterValidatorAfk() external { 72: require(!validatorWhitelistDisabled, "WHITELIST_DISABLED"); 74: validatorWhitelistDisabled = true;
Consider whether reasonable bounds checks for variables would be useful.
There are 6 instances:
328: challengePeriodBlocks = _challengePeriodBlocks; 339: LAYERZERO_BLOCKEDGE_HEIGHT = layerZeroBlockEdgeHeight; 343: LAYERZERO_BIGSTEPEDGE_HEIGHT = layerZeroBigStepEdgeHeight; 347: LAYERZERO_SMALLSTEPEDGE_HEIGHT = layerZeroSmallStepEdgeHeight;
205: minimumAssertionPeriod = newPeriod; 224: baseStake = newBaseStake;
Modern smart contract development often employs upgradeable contract structures, utilizing proxy patterns like OpenZeppelin’s Upgradeable Contracts. This paradigm separates logic and state, allowing developers to amend and enhance the contract's functionality without altering its state or the deployed contract address. Transitioning to this approach enhances long-term maintainability. Resolution: Adopt a well-established proxy pattern for upgradeability, ensuring proper initialization and employing transparent proxies to mitigate potential risks. Embrace comprehensive testing and audit practices, particularly when updating contract logic, to ensure state consistency and security are preserved across upgrades. This ensures your contract remains robust and adaptable to future requirements.
<details> <summary>There are 11 instances (click to show):</summary>21: contract AssertionStakingPool is AbsBoldStakingPool, IAssertionStakingPool {
13: contract AssertionStakingPoolCreator is IAssertionStakingPoolCreator {
25: contract EdgeStakingPool is AbsBoldStakingPool, IEdgeStakingPool {
13: contract EdgeStakingPoolCreator is IEdgeStakingPoolCreator {
64: contract SequencerInbox is DelegateCallAware, GasRefundEnabled, ISequencerInbox {
94: contract StateHashPreImageLookup { 123: contract RollupReader is IOldRollup { 166: contract ConstantArrayStorage { 182: contract BOLDUpgradeAction {
21: contract BridgeCreator is Ownable {
</details>17: contract RollupCreator is Ownable {
This includes: large code bases, or code with lots of inline-assembly, complicated math, or complicated interactions between multiple contracts. Invariant fuzzers such as Echidna require the test writer to come up with invariants which should not be violated under any circumstances, and the fuzzer tests various inputs and function calls to ensure that the invariants always hold. Even code with 100% code coverage can still have bugs due to the order of the operations a user performs, and invariant fuzzers may help significantly.
There is 1 instance:
In instances where a new variable is defined, there is no need to set it to it's default value.
<details> <summary>There are 23 instances (click to show):</summary>85: bytes1 public constant BROTLI_MESSAGE_HEADER_FLAG = 0x00; 317: bytes32 prevDelayedAcc = 0;
492: for (uint256 i = 0; i < edgeIds.length; i++) { 517: uint64 assertionBlocks = 0;
15: for (uint256 i = 0; i < arr.length; i++) { 47: for (uint256 i = 0; i < arr1.length; i++) { 50: for (uint256 i = 0; i < arr2.length; i++) {
114: bytes32 accum = 0; 115: for (uint256 i = 0; i < me.length; i++) { 188: for (uint256 i = 0; i < me.length; i++) { 293: uint256 sum = 0; 294: for (uint256 i = 0; i < me.length; i++) { 326: uint256 proofIndex = 0;
350: for (uint64 i = 0; i < stakerCount; i++) { 528: for (uint256 i = 0; i < validators.length; i++) {
63: bytes32 parentAssertionHash = bytes32(0); 64: bytes32 inboxAcc = bytes32(0); 184: for (uint256 i = 0; i < _validator.length; i++) { 230: for (uint256 i = 0; i < staker.length; i++) {
523: bytes32 parentAssertionHash = bytes32(0); 524: bytes32 inboxAcc = bytes32(0);
</details>225: for (uint256 i = 0; i < deployParams.batchPosters.length; i++) { 235: for (uint256 i = 0; i < deployParams.validators.length; i++) {
public/external functions exposed by interfacesAll external/public functions should extend an interface. This is useful to ensure that the whole API is extracted and can be more easily integrated by other projects.
64: contract SequencerInbox is DelegateCallAware, GasRefundEnabled, ISequencerInbox {
94: contract StateHashPreImageLookup { 166: contract ConstantArrayStorage { 182: contract BOLDUpgradeAction {
21: contract BridgeCreator is Ownable {
15: contract RollupAdminLogic is RollupCore, IRollupAdmin, DoubleLogicUUPSUpgradeable {
17: contract RollupCreator is Ownable {
11: contract RollupProxy is AdminFallbackProxy {
</details>15: contract RollupUserLogic is RollupCore, UUPSNotUpgradeable, IRollupUser {
5: pragma solidity ^0.8.0; 6: 7: import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
6: pragma solidity ^0.8.0; 7: 8: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
6: pragma solidity ^0.8.0; 7: 8: import "./AssertionStakingPool.sol";
5: pragma solidity ^0.8.0; 6: 7: import "./AbsBoldStakingPool.sol";
6: pragma solidity ^0.8.0; 7: 8: import "./EdgeStakingPool.sol";
6: pragma solidity ^0.8.0; 7: 8: import "@openzeppelin/contracts/utils/Create2.sol"; 9: import "@openzeppelin/contracts/utils/Address.sol"; 10: 11: library StakingPoolCreatorUtils {
5: pragma solidity ^0.8.0; 6: 7: interface IAbsBoldStakingPool {
5: pragma solidity ^0.8.0; 6: 7: import "../../rollup/IRollupLogic.sol"; 8: import "./IAbsBoldStakingPool.sol"; 9: 10: interface IAssertionStakingPool is IAbsBoldStakingPool {
5: pragma solidity ^0.8.0; 6: 7: import "../../rollup/IRollupLogic.sol"; 8: import "./IAssertionStakingPool.sol"; 9: 10: interface IAssertionStakingPoolCreator {
5: pragma solidity ^0.8.0; 6: 7: import "../../challengeV2/EdgeChallengeManager.sol"; 8: import "./IAbsBoldStakingPool.sol"; 9: 10: interface IEdgeStakingPool is IAbsBoldStakingPool {
5: pragma solidity ^0.8.0; 6: 7: import "./IEdgeStakingPool.sol"; 7: import "./IEdgeStakingPool.sol"; 8: 9: interface IEdgeStakingPoolCreator {
5: pragma solidity ^0.8.0; 6: 7: import "./Messages.sol"; 106: } 107: 108: function isUpdatable(BufferData storage self) internal view returns (bool) { 113: } 114: 115: function isValidBufferConfig(BufferConfig memory config) internal pure returns (bool) {
7: pragma experimental ABIEncoderV2; 8: 9: import "../libraries/IGasRefunder.sol"; 13: import "./DelayBufferTypes.sol"; 14: 15: interface ISequencerInbox is IDelayedMessageProvider {
5: pragma solidity ^0.8.0; 6: 7: import { 106: } 107: 108: modifier onlyRollupOwnerOrBatchPosterManager() { 155: } 156: 157: function _chainIdChanged() internal view returns (bool) { 159: } 160: 161: function postUpgradeInit(BufferConfig memory bufferConfig_) 175: } 176: 177: function initialize( 214: } 215: 216: function getTimeBounds() internal view virtual returns (IBridge.TimeBounds memory) { 242: } 243: 244: function maxTimeVariation() 267: } 268: 269: function maxTimeVariationInternal() 453: } 454: 455: function addSequencerL2BatchFromBlobsImpl( 510: } 511: 512: function addSequencerL2BatchFromCalldataImpl( 603: } 604: 605: function delayProofImpl(uint256 afterDelayedMessagesRead, DelayProof memory delayProof) 626: } 627: 628: function isDelayProofRequired(uint256 afterDelayedMessagesRead) internal view returns (bool) { 635: } 636: 637: function packHeader(uint256 afterDelayedMessagesRead) 789: } 790: 791: function addSequencerL2BatchImpl( 822: } 823: 824: function inboxAccs(uint256 index) external view returns (bytes32) { 826: } 827: 828: function batchCount() external view returns (uint256) { 845: } 846: 847: function _setBufferConfig(BufferConfig memory bufferConfig_) internal { 865: } 866: 867: function _setMaxTimeVariation(ISequencerInbox.MaxTimeVariation memory maxTimeVariation_) 945: } 946: 947: function setBufferConfig(BufferConfig memory bufferConfig_) external onlyRollupOwner { 950: } 951: 952: function isValidKeysetHash(bytes32 ksHash) external view returns (bool) {
5: pragma solidity ^0.8.17; 6: 7: import "../rollup/Assertion.sol";
5: pragma solidity ^0.8.17; 6: 7: import "../bridge/IBridge.sol";
5: pragma solidity ^0.8.17; 6: 7: import "./Enums.sol"; 65: } 66: 67: library ChallengeEdgeLib { 182: } 183: 184: function mutualIdMem(ChallengeEdge memory ce) internal pure returns (bytes32) {
5: pragma solidity ^0.8.17; 6: 7: import "./Enums.sol";
5: pragma solidity ^0.8.17; 6: 7: import "./UintUtilsLib.sol"; 486: } 487: 488: function timeUnrivaledTotal(EdgeStore storage store, bytes32 edgeId) internal view returns (uint256) { 514: } 515: 516: function updateTimerCacheByChildren(EdgeStore storage store, bytes32 edgeId) internal returns (bool, uint256) { 518: } 519: 520: function updateTimerCacheByClaim(
5: pragma solidity ^0.8.17; 6: 7: import "../../libraries/MerkleLib.sol";
5: pragma solidity ^0.8.0; 6: 7: import "./AssertionState.sol"; 91: } 92: 93: function requireExists(AssertionNode memory self) internal pure {
5: pragma solidity ^0.8.0; 6: 7: import "../state/GlobalState.sol"; 15: } 16: 17: library AssertionStateLib { 20: } 21: 22: function hash(AssertionState memory state) internal pure returns (bytes32) {
5: pragma solidity ^0.8.0; 6: 7: import "@openzeppelin/contracts-upgradeable/utils/Create2Upgradeable.sol"; 47: } 48: 49: interface IOldRollup { 75: } 76: 77: interface IOldRollupAdmin { 81: } 82: 83: interface ISeqInboxPostUpgradeInit { 104: } 105: 106: function set(bytes32 h, ExecutionState calldata executionState, uint256 inboxMaxCount) public { 110: } 111: 112: function get(bytes32 h) public view returns (ExecutionState memory executionState, uint256 inboxMaxCount) { 128: } 129: 130: function wasmModuleRoot() external view returns (bytes32) { 132: } 133: 134: function latestConfirmed() external view returns (uint64) { 136: } 137: 138: function getNode(uint64 nodeNum) external view returns (Node memory) { 140: } 141: 142: function getStakerAddress(uint64 stakerNum) external view returns (address) { 144: } 145: 146: function stakerCount() external view returns (uint64) { 148: } 149: 150: function getStaker(address staker) external view returns (OldStaker memory) { 152: } 153: 154: function isValidator(address validator) external view returns (bool) { 156: } 157: 158: function validatorWalletCreator() external view returns (address) { 171: } 172: 173: function array() public view returns (uint256[] memory) { 409: } 410: 411: function upgradeSurroundingContracts(address newRollupAddress) private { 431: } 432: 433: function upgradeSequencerInbox() private { 462: } 463: 464: function perform(address[] memory validators) external {
5: pragma solidity ^0.8.0; 6: 7: import "../bridge/Bridge.sol"; 19: import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; 20: 21: contract BridgeCreator is Ownable { 51: } 52: 53: function updateTemplates(BridgeTemplates calldata _newTemplates) external onlyOwner { 56: } 57: 58: function updateERC20Templates(BridgeTemplates calldata _newTemplates) external onlyOwner { 61: } 62: 63: function _createBridge( 97: } 98: 99: function createBridge(
5: pragma solidity ^0.8.0; 6: 7: import "../state/GlobalState.sol";
5: pragma solidity ^0.8.0; 6: 7: import "./IRollupCore.sol"; 11: import "./Config.sol"; 12: 13: interface IRollupAdmin {
5: pragma solidity ^0.8.0; 6: 7: import "./Assertion.sol"; 13: import "../challengeV2/IAssertionChain.sol"; 14: 15: interface IRollupCore is IAssertionChain {
5: pragma solidity ^0.8.0; 6: 7: import "./IRollupCore.sol"; 10: import "../bridge/IOwnable.sol"; 11: 12: interface IRollupUser is IRollupCore, IOwnable {
5: pragma solidity ^0.8.0; 6: 7: import "./IRollupAdmin.sol"; 13: import "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol"; 14: 15: contract RollupAdminLogic is RollupCore, IRollupAdmin, DoubleLogicUUPSUpgradeable { 226: } 227: 228: function forceRefundStaker(address[] calldata staker) external override whenPaused { 235: } 236: 237: function forceCreateAssertion( 256: } 257: 258: function forceConfirmAssertion( 267: } 268: 269: function setLoserStakeEscrow(address newLoserStakerEscrow) external override {
5: pragma solidity ^0.8.0; 6: 7: import "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol"; 20: import "../libraries/ArbitrumChecker.sol"; 21: 22: abstract contract RollupCore is IRollupCore, PausableUpgradeable { 363: } 364: 365: function createNewAssertion( 518: } 519: 520: function genesisAssertionHash() external pure returns (bytes32) { 530: } 531: 532: function getFirstChildCreationBlock(bytes32 assertionHash) external view returns (uint64) { 534: } 535: 536: function getSecondChildCreationBlock(bytes32 assertionHash) external view returns (uint64) { 538: } 539: 540: function validateAssertionHash( 547: } 548: 549: function validateConfig(bytes32 assertionHash, ConfigData calldata configData) external view { 551: } 552: 553: function isFirstChild(bytes32 assertionHash) external view returns (bool) { 555: } 556: 557: function isPending(bytes32 assertionHash) external view returns (bool) {
5: pragma solidity ^0.8.0; 6: 7: import "./RollupProxy.sol"; 15: import {SafeERC20, IERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; 16: 17: contract RollupCreator is Ownable { 61: receive() external payable {} 62: 63: function setTemplates( 265: } 266: 267: function _deployUpgradeExecutor(address rollupOwner, ProxyAdmin proxyAdmin) 285: } 286: 287: function _deployFactories(
5: pragma solidity ^0.8.0; 6: 7: import "../state/GlobalState.sol"; 15: import "../challengeV2/EdgeChallengeManager.sol"; 16: 17: library RollupLib { 71: } 72: 73: function validateConfigHash(
5: pragma solidity ^0.8.0; 6: 7: import "../libraries/AdminFallbackProxy.sol"; 9: import "./Config.sol"; 10: 11: contract RollupProxy is AdminFallbackProxy {
</details>5: pragma solidity ^0.8.0; 6: 7: import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; 13: import {ETH_POS_BLOCK_TIME} from "../libraries/Constants.sol"; 14: 15: contract RollupUserLogic is RollupCore, UUPSNotUpgradeable, IRollupUser { 60: } 61: 62: function removeWhitelistAfterFork() external { 306: } 307: 308: function owner() external view returns (address) { 364: } 365: 366: function receiveTokens(uint256 tokenAmount) private {
Formal verification is the act of proving or disproving the correctness of intended algorithms underlying a system with respect to a certain formal specification/property/invariant, using formal methods of mathematics.
Some tools that are currently available to perform these tests on smart contracts are SMTChecker and Certora Prover.
<details> <summary>There are 39 instances (click to show):</summary>The use of scoped blocks, denoted by {} without a preceding control structure like if, for, etc., allows for the creation of isolated scopes within a function. While this can be useful for managing memory and preventing naming conflicts, it should be used sparingly. Excessive use of these scope blocks can obscure the code's logic flow and make it more difficult to understand, impeding code maintainability. As a best practice, only employ scoped blocks when necessary for memory management or to avoid clear naming conflicts. Otherwise, aim for clarity and simplicity in your code structure for optimal readability and maintainability.
There are 6 instances:
622: { 623: (bytes32[] memory preExpansion, bytes32[] memory proof) = abi.decode(prefixProof, (bytes32[], bytes32[])); 631: { 632: // midpoint proof it valid, create and store the children 645: { 646: ChallengeEdge memory upperChild = ChallengeEdgeLib.newChildEdge( 797: { 798: bytes32 cursor = edgeId;
405: { 406: // This new assertion consumes the messages from prevInboxPosition to afterInboxPosition
142: { 143: // Make sure the immutable maxDataSize is as expected
#0 - c4-judge
2024-06-05T11:40:58Z
Picodes marked the issue as grade-b