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: 19/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
| Number | Details |
|---|---|
| [Low-1] | The funds will get lost when a non-staker calls addToDeposit |
| [Low-2] | reduceDeposit function can´t be called from AssertionStakingPool contract |
| [Low-3] | AssertionStakingPool can only create one stake as contrary to the stakeOnNewAssertion function |
| [Low-4] | _stakeToken is not validated properly in RollupUserLogic contract |
| [Low-5] | cleanRollUp function doesn´t refund all stakers |
| [Low-6] | forceRefundStaker logic is not complying with the intented flow |
| [Low-7] | Several setter functions are not implemented |
| [Low-8] | Missing validatorWhitelistDisabled event will shadow which function is called on the global level |
| [Low-9] | Missing checks for address(0x0) when updating address state variables |
| [Low-10] | Missing empty bytes check when assigning bytes/string state variable |
| [Low-11] | Contracts with multiple onlyXYZ modifiers where XYZ is a role can introduce complexities when managing privileges |
| [Low-12] | Require should be used instead of assert as assert in some solidity versions does not refund remaining gas |
| [Low-13] | Experimental functionality should not be used in production code |
| [Low-14] | Ownable2Step should be used in place of Ownable |
| [Low-15] | Upgradable contracts should have a __gap variable |
| [Low-16] | Loss of precision |
| [Low-17] | Missing zero address check in constructor |
| [Low-18] | Use of onlyOwner functions can be lost |
| [Low-19] | Vulnerable version of openzeppelin contracts used |
| [Low-20] | State variables not capped at reasonable values |
| Number | Details |
|---|---|
| [NonCritical-1] | Greater than comparisons made on state uints that can be set to max |
| [NonCritical-2] | Public or external initialize functions should be protected with the initializer modifier |
| [NonCritical-3] | Double imports in solidity file |
| [NonCritical-4] | Critical functions should have a timelock |
| [NonCritical-5] | Consider implementing two-step procedure for updating protocol addresses |
| [NonCritical-6] | Floating pragma should be avoided |
| [NonCritical-7] | Reverts should use custom errors instead of strings |
| [NonCritical-8] | Functions with array parameters should have length checks in place |
| [NonCritical-9] | Defined named returns not used within function |
| [NonCritical-10] | Missing events in sensitive functions |
| [NonCritical-11] | Avoid mutating function parameters |
| [NonCritical-12] | A event should be emitted if a non immutable state variable is set in a constructor |
| [NonCritical-13] | Immutable and constant integer state variables should not be casted |
| [NonCritical-14] | Use transfer libraries instead of low level calls |
| [NonCritical-15] | Inconsistent checks of address params against address(0) |
| [NonCritical-16] | Avoid declaring variables with the names of defined functions within the project |
| [NonCritical-17] | int/uint passed into abi.encodePacked without casting to a string. |
| [NonCritical-18] | For loop iterates on arrays not indexed |
| [NonCritical-19] | Avoid arithmetic directly within array indices |
| [NonCritical-20] | Modifier checks msg.sender against two addresses. Consider using multiple modifiers for more control |
addToDepositaddToDeposit is implemented in RollupUserLogic to increase the amount staked tokens for the given staker
Function is below:
Contract: RollupUserLogic.sol 381: function addToDeposit(address stakerAddress, uint256 tokenAmount) 382: external 383: onlyValidator 384: whenNotPaused 385: { 386: _addToDeposit(stakerAddress, tokenAmount); 387: /// @dev This is an external call, safe because it's at the end of the function 388: receiveTokens(tokenAmount); 389: }
And it internally calls _addToDeposit as below:
Contract: RollupUserLogic.sol 253: function _addToDeposit(address stakerAddress, uint256 depositAmount) 254: internal 255: onlyValidator 256: whenNotPaused 257: { 258: require(isStaked(stakerAddress), "NOT_STAKED"); 259: increaseStakeBy(stakerAddress, depositAmount); 260: } 261:
And finally the function calls increaseStakeBy;
Contract: RollupCore.sol 310: function increaseStakeBy(address stakerAddress, uint256 amountAdded) internal { 311: Staker storage staker = _stakerMap[stakerAddress]; 312: uint256 initialStaked = staker.amountStaked; 313: uint256 finalStaked = initialStaked + amountAdded; 314: staker.amountStaked = finalStaked; 315: emit UserStakeUpdated(stakerAddress, staker.withdrawalAddress, initialStaked, finalStaked); 316: } 317:
The whole process is intended to increase the staker amount and it´s not validated whether the caller has the stake but only onlyValidator modifier.
And onlyValidator modifier is below:
Contract: RollupUserLogic.sol 20: modifier onlyValidator() { 21: require(isValidator[msg.sender] || validatorWhitelistDisabled, "NOT_VALIDATOR"); 22: _; 23: }
This means everyone could call this once the white list is disabled.
But once anyone - who doesn´t have active stake - calls this with the intention of increasing the stake of the staker, their funds will get lost - donated to the staker (if honest staker) or lost (if dishonest staker) since this deposit is not accounted as in AbsBoldStakingPool.
We recommend validating the caller and accounting the deposit amount if the caller calls this for an active stake where the staker address is not msg.sender
reduceDeposit function can´t be called from AssertionStakingPool contractThe function is intended to be called to reduce the staked amount where the staker has surplus staked amounts.
Contract: RollupUserLogic.sol 262: /** 263: * @notice Reduce the amount staked for the sender (difference between initial amount staked and target is creditted back to the sender). 264: * @param target Target amount of stake for the staker. 265: */ 266: function reduceDeposit(uint256 target) external onlyValidator whenNotPaused { 267: requireInactiveStaker(msg.sender); 268: // amount will be checked when creating an assertion 269: reduceStakeTo(msg.sender, target); 270: }
However, it´s not provisioned to the AssertionStakingPool contract and it´s contrary to the NATSPEC at L:191 inside the stakeOnNewAssertion function:
Contract: RollupUserLogic.sol 191: // excess stake can be removed by calling reduceDeposit when the staker is inactive
AssertionStakingPool can only create one stake as contrary to the stakeOnNewAssertion functionstakeOnNewAssertion function is called to create a new assertion and move stake onto it.
It also allows the staker to call it again with the conditions as per the comments:
Contract: RollupUserLogic.sol 204: // Staker can create new assertion only if 205: // a) its last staked assertion is the prev; or 206: // b) its last staked assertion have a child
But this option is not provisioned to the stakes originated from AssertionStakingPool
_stakeToken is not validated properly in RollupUserLogic contractThe inheritance of RollupAdminLogic, RollupCore and RollupUserLogic is follows:
RollupAdminLogic is RollupCore RollupUserLogic is RollupCore RollupCore is IRollupCore
And all the variables of RollupCore is set in the RollupAdminLogic´s initialize function with Config calldata config
The stakeToken is set accordingly.
While initializing the RollupUserLogic contract, it´s validating the stakeToken not being zero.
However, it´s not calling the correct variable, as it´s only validating the memory parameter is not being zero.
Contract: RollupUserLogic.sol 25: /// @dev the user logic just validated configuration and shouldn't write to state during init 26: /// this allows the admin logic to ensure consistency on parameters. 27: function initialize(address _stakeToken) external view override onlyProxy { 28: //via delegatecall 29: require(_stakeToken != address(0), "NEED_STAKE_TOKEN"); 30: }
cleanRollUp function doesn´t refund all stakersWhile there´s a comment of number of staker expactancy, it could be the opposite when the baseStake is reduced in the upcoming upgrades.
As can be seen that, the function just erases the stakers after No:50,
Contract: BOLDUpgradeAction.sol 341: function cleanupOldRollup() private { 342: IOldRollupAdmin(address(OLD_ROLLUP)).pause(); 343: 344: uint64 stakerCount = ROLLUP_READER.stakerCount(); 345: // since we for-loop these stakers we set an arbitrary limit - we dont 346: // expect any instances to have close to this number of stakers 347: if (stakerCount > 50) { 348: stakerCount = 50; 349: } 350: for (uint64 i = 0; i < stakerCount; i++) { 351: address stakerAddr = ROLLUP_READER.getStakerAddress(i); 352: OldStaker memory staker = ROLLUP_READER.getStaker(stakerAddr); 353: if (staker.isStaked && staker.currentChallenge == 0) { 354: address[] memory stakersToRefund = new address[](1); 355: stakersToRefund[0] = stakerAddr; 356: 357: IOldRollupAdmin(address(OLD_ROLLUP)).forceRefundStaker(stakersToRefund); 358: } 359: } 360: 361: // upgrade the rollup to one that allows validators to withdraw even whilst paused 362: DoubleLogicUUPSUpgradeable(address(OLD_ROLLUP)).upgradeSecondaryTo(IMPL_PATCHED_OLD_ROLLUP_USER); 363: }
However, this could be cached and executed in multi-steps.
forceRefundStaker logic is not complying with the intented flowforceRefundStaker is called in case of wasm module bug as per the comments;
Contract: RollupAdminLogic.sol 242: // To update the wasm module root in the case of a bug: 243: // 0. pause the contract 244: // 1. update the wasm module root in the contract 245: // 2. update the config hash of the assertion after which you wish to use the new wasm module root (functionality not written yet) 246: // 3. force refund the stake of the current leaf assertion(s) 247: // 4. create a new assertion using the assertion with the updated config has as a prev 248: // 5. force confirm it - this is necessary to set latestConfirmed on the correct line 249: // 6. unpause the contract
As L:246 states, the function is called accordingly.
However, when the wasm module malfunctions and the honest staker´s fund stuck accordingly due to losing the edge in OSP well before - previous leaf, they won´t be passing the requireInactiveStaker call
Contract: RollupCore.sol 614: function requireInactiveStaker(address stakerAddress) internal view { 615: require(isStaked(stakerAddress), "NOT_STAKED"); 616: // A staker is inactive if 617: // a) their last staked assertion is the latest confirmed assertion 618: // b) their last staked assertion have a child 619: bytes32 lastestAssertion = latestStakedAssertion(stakerAddress); 620: bool isLatestConfirmed = lastestAssertion == latestConfirmed(); 621: bool haveChild = getAssertionStorage(lastestAssertion).firstChildBlock > 0; 622: require(isLatestConfirmed || haveChild, "STAKE_ACTIVE"); 623: }
As can be seen that they won´t suffice the requirement for a and b
We recommend a new function where it doesnt´t check the requireInactiveStaker for those instances.
While we´re aware of that the contracts are upgradable, we also think that it should NOT be necessary to upgrade the whole contract to change a setting where it might be needed in the future due to various reasons - mostly dependencies. The variables below don´t have any setter functions:
Contract: EdgeChallengeManager.sol 279: uint256[] public stakeAmounts; 282: uint64 public challengePeriodBlocks;
Contract: RollupCore.sol 93: address public stakeToken; 300: withdrawalAddress
While the staker´s withdrawalAddress is not a state variable, we think that the users should have the ability to change their withdrawal address as loss of wallets are common in the wild.
validatorWhitelistDisabled event will shadow which function is called on the global levelBelow should emit the event to see which function triggers the whitelistdisablement if both happens in the same time range.
Contract: RollupUserLogic.sol 62: 63: function removeWhitelistAfterFork() external { 64: 65: require(!validatorWhitelistDisabled, "WHITELIST_DISABLED"); 66: require(_chainIdChanged(), "CHAIN_ID_NOT_CHANGED"); 67: validatorWhitelistDisabled = true; 68: } 69: 70: /** 71: * @notice Remove the whitelist after the validator has been inactive for too long 72: */ 73: function removeWhitelistAfterValidatorAfk() external { 74: require(!validatorWhitelistDisabled, "WHITELIST_DISABLED"); 75: require(_validatorIsAfk(), "VALIDATOR_NOT_AFK"); 76: validatorWhitelistDisabled = true; 77: }
Num of instances: 4
['63']
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 { 73: bridgeCreator = _bridgeCreator; 74: osp = _osp; 75: challengeManagerTemplate = _challengeManagerLogic; 76: rollupAdminLogic = _rollupAdminLogic; 77: rollupUserLogic = _rollupUserLogic; 78: upgradeExecutorLogic = _upgradeExecutorLogic; 79: validatorWalletCreator = _validatorWalletCreator; 80: l2FactoriesDeployer = _l2FactoriesDeployer; 81: emit TemplatesUpdated(); 82: }
['894']
894: function setIsBatchPoster(address addr, bool isBatchPoster_) 895: external 896: onlyRollupOwnerOrBatchPosterManager 897: { 898: isBatchPoster[addr] = isBatchPoster_; 899: emit OwnerFunctionCalled(1); 900: }
['933']
</details>933: function setIsSequencer(address addr, bool isSequencer_) 934: external 935: onlyRollupOwnerOrBatchPosterManager 936: { 937: isSequencer[addr] = isSequencer_; 938: emit OwnerFunctionCalled(4); 939: }
In programming, especially when dealing with dynamically sized types like bytes and strings in Solidity, omitting a check for empty values when assigning to a state variable can lead to unexpected behavior or vulnerabilities. This oversight may allow zero-length or null values to be stored, which could be problematic in contexts where valid data is expected. Implementing explicit checks for empty bytes or strings before assignment ensures data integrity and can prevent potential logic errors or exploits in smart contracts. It's a crucial practice in robust smart contract development to validate data thoroughly before state modifications.
Num of instances: 2
['281']
281: function setWasmModuleRoot(bytes32 newWasmModuleRoot) external override { 282: wasmModuleRoot = newWasmModuleRoot; // <= FOUND 283: emit OwnerFunctionCalled(26); 284: }
['225']
</details>225: function initializeCore(AssertionNode memory initialAssertion, bytes32 assertionHash) internal { 226: __Pausable_init(); 227: initialAssertion.status = AssertionStatus.Confirmed; 228: _assertions[assertionHash] = initialAssertion; 229: _latestConfirmed = assertionHash; // <= FOUND 230: }
In smart contracts, using multiple onlyXYZ modifiers for different roles can complicate privilege management. OpenZeppelin's AccessControl offers a streamlined solution, enabling easier and more flexible role-based permission handling. It simplifies the assignment and revocation of roles compared to multiple individual modifiers, reducing potential errors and improving contract maintainability. This modular approach to access management makes it more straightforward to define, manage, and audit roles and permissions within a contract.
Num of instances: 1
['64']
</details>64: contract SequencerInbox is DelegateCallAware, GasRefundEnabled, ISequencerInbox { 65: uint256 public totalDelayedMessagesRead; 66: 103: modifier onlyRollupOwner() { // <= FOUND 104: if (msg.sender != rollup.owner()) revert NotOwner(msg.sender, rollup.owner()); 105: _; 106: } 107: 108: modifier onlyRollupOwnerOrBatchPosterManager() { // <= FOUND 109: if (msg.sender != rollup.owner() && msg.sender != batchPosterManager) { 108: modifier onlyRollupOwnerOrBatchPosterManager() { // <= FOUND 109: if (msg.sender != rollup.owner() && msg.sender != batchPosterManager) { 110: revert NotBatchPosterManager(msg.sender); 111: } 112: _; 113: } 114:
In Solidity, require should be used instead of assert for input validation and error handling due to its more efficient and flexible nature. While both functions check for a specified condition and revert the transaction if it evaluates to false, assert consumes all remaining gas upon failure, which can lead to higher costs for users. On the other hand, require refunds unused gas, making it a more gas-efficient option. Additionally, require allows for custom error messages, which improves debugging and error reporting.
Num of instances: 2
['637']
637: function packHeader(uint256 afterDelayedMessagesRead) 638: internal 639: view 640: returns (bytes memory, IBridge.TimeBounds memory) 641: { 642: IBridge.TimeBounds memory timeBounds = getTimeBounds(); 643: bytes memory header = abi.encodePacked( 644: timeBounds.minTimestamp, 645: timeBounds.maxTimestamp, 646: timeBounds.minBlockNumber, 647: timeBounds.maxBlockNumber, 648: uint64(afterDelayedMessagesRead) 649: ); 650: 651: assert(header.length == HEADER_LENGTH); // <= FOUND 652: return (header, timeBounds); 653: }
['312']
</details>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 { 320: require(preSize > 0, "Pre-size cannot be 0"); 321: require(root(preExpansion) == preRoot, "Pre expansion root mismatch"); 322: require(treeSize(preExpansion) == preSize, "Pre size does not match expansion"); 323: require(preSize < postSize, "Pre size not less than post size"); 324: 325: uint256 size = preSize; 326: uint256 proofIndex = 0; 327: 328: 329: bytes32[] memory exp = ArrayUtilsLib.slice(preExpansion, 0, preExpansion.length); 330: 331: 332: while (size < postSize) { 333: uint256 level = maximumAppendBetween(size, postSize); 334: 335: require(proofIndex < proof.length, "Index out of range"); 336: exp = appendCompleteSubTree(exp, level, proof[proofIndex]); 337: 338: uint256 numLeaves = 1 << level; 339: size += numLeaves; 340: assert(size <= postSize); // <= FOUND 341: proofIndex++; 342: } 343: 344: 345: require(root(exp) == postRoot, "Post expansion root not equal post"); 346: 347: 348: 349: require(proofIndex == proof.length, "Incomplete proof usage"); 350: }
Experimental pragma features should not be used within production code due to their unstable and untested nature. These features are still under development and have not undergone rigorous scrutiny, making them susceptible to bugs, security vulnerabilities, and potential breaking changes in future Solidity releases.
Num of instances: 1
['7']
</details>7: pragma experimental ABIEncoderV2; // <= FOUND
Ownable2Step further prevents risks posed by centralised privileges as there is a smaller likelihood of the owner being wrongfully changed
Num of instances: 2
['17']
17: contract RollupCreator is Ownable // <= FOUND
['21']
</details>21: contract BridgeCreator is Ownable // <= FOUND
This is to ensure the team can add new state variables without compromising compatability.
Num of instances: 4
['206']
206: contract EdgeChallengeManager is IEdgeChallengeManager, Initializable
['15']
15: contract RollupUserLogic is RollupCore, UUPSNotUpgradeable, IRollupUser
['15']
15: contract RollupAdminLogic is RollupCore, IRollupAdmin, DoubleLogicUUPSUpgradeable
['22']
</details>22: abstract contract RollupCore is IRollupCore, PausableUpgradeable
Dividing by large numbers in Solidity can cause a loss of precision due to the language's inherent integer division behavior. Solidity does not support floating-point arithmetic, and as a result, division between integers yields an integer result, truncating any fractional part. When dividing by a large number, the resulting value may become significantly smaller, leading to a loss of precision, as the fractional part is discarded.
Num of instances: 1
['33']
</details>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) { 42: uint256 elapsed = end > start ? end - start : 0; 43: uint256 delay = sequenced > start ? sequenced - start : 0; 44: 45: 46: buffer += (elapsed * replenishRateInBasis) / BASIS; 47: 48: uint256 unexpectedDelay = delay > threshold ? delay - threshold : 0; 49: if (unexpectedDelay > elapsed) { 50: unexpectedDelay = elapsed; 51: } 52: 53: 54: if (buffer > unexpectedDelay) { 55: buffer -= unexpectedDelay; 56: if (buffer > threshold) { 57: 58: return buffer > max ? max : buffer; 59: } 60: } 61: 62: return threshold; 63: }
In Solidity, constructors often take address parameters to initialize important components of a contract, such as owner or linked contracts. However, without a check, there's a risk that an address parameter could be mistakenly set to the zero address (0x0). This could occur due to a mistake or oversight during contract deployment. A zero address in a crucial role can cause serious issues, as it cannot perform actions like a normal address, and any funds sent to it are irretrievable. Therefore, it's crucial to include a zero address check in constructors to prevent such potential problems. If a zero address is detected, the constructor should revert the transaction.
Num of instances: 2
['35']
35: constructor( 36: address _challengeManager, // <= FOUND 37: bytes32 _edgeId 38: ) AbsBoldStakingPool(address(EdgeChallengeManager(_challengeManager).stakeToken())) { 39: challengeManager = _challengeManager; 40: edgeId = _edgeId; 41: }
['31']
</details>31: constructor( 32: address _rollup, // <= FOUND 33: bytes32 _assertionHash 34: ) AbsBoldStakingPool(IRollupCore(_rollup).stakeToken()) { 35: rollup = _rollup; 36: assertionHash = _assertionHash; 37: }
In Solidity, renouncing ownership of a contract essentially transfers ownership to the zero address. This is an irreversible operation and has considerable security implications. If the renounceOwnership function is used, the contract will lose the ability to perform any operations that are limited to the owner. This can be problematic if there are any bugs, flaws, or unexpected events that require owner intervention to resolve. Therefore, in some instances, it is better to disable or omit the renounceOwnership function, and instead implement a secure transferOwnership function. This way, if necessary, ownership can be transferred to a new, trusted party without losing the potential for administrative intervention.
Num of instances: 2
['17']
17: contract RollupCreator is Ownable // <= FOUND
['21']
</details>21: contract BridgeCreator is Ownable // <= FOUND
OpenZeppelin versions of 4.9.2 and below are vulnerable to exploits, please consider upgrading to 4.9.3 or above. See: https://security.snyk.io/package/npm/@openzeppelin%2Fcontracts for more details
Num of instances: 1
['1']
</details>1: { 2: "name": "@arbitrum/bold-contracts", 3: "version": "0.0.1", 4: "description": "Layer 2 precompiles and rollup for Arbitrum Nitro with BOLD", 5: "author": "Offchain Labs, Inc.", 6: "license": "BUSL-1.1", 7: "repository": { 8: "type": "git", 9: "url": "git+https://github.com/offchainlabs/bold.git" 10: }, 11: "files": [ 12: "src/", 13: "build/contracts/src", 14: "build/contracts/@openzeppelin", 15: "out/yul/Reader4844.yul/Reader4844.json" 16: ], 17: "bugs": { 18: "url": "https://github.com/offchainlabs/bold/issues" 19: }, 20: "scripts": { 21: "prepublishOnly": "hardhat clean && forge clean && hardhat compile && yarn build:forge:yul", 22: "coverage": "forge coverage --report lcov --ir-minimum && lcov --remove lcov.info 'node_modules/*' 'test/*' 'script/*' 'src/test-helpers/*' 'challenge/*' --ignore-errors unused -o lcov.info && genhtml lcov.info --branch-coverage --output-dir coverage", 23: "build:all": "yarn build && yarn build:forge", 24: "build": "hardhat compile", 25: "build:forge:sol": "forge build --skip *.yul", 26: "build:forge:yul": "FOUNDRY_PROFILE=yul forge build --skip *.sol", 27: "build:forge": "yarn build:forge:sol && yarn build:forge:yul", 28: "contract:size": "STRICT=true hardhat size-contracts", 29: "lint:test": "eslint ./test", 30: "solhint": "solhint -f table src/**/*.sol", 31: "prettier:solidity": "prettier --write src/**/*.sol", 32: "format": "prettier './**/*.{js,json,md,ts,yml,sol}' --write && yarn run lint:test --fix", 33: "build:0.6": "INTERFACE_TESTER_SOLC_VERSION=0.6.9 yarn run build", 34: "build:0.7": "INTERFACE_TESTER_SOLC_VERSION=0.7.0 yarn run build", 35: "test": "DISABLE_GAS_REPORTER=true hardhat --network hardhat test test/contract/*.spec.ts", 36: "test:4844": "DISABLE_GAS_REPORTER=true hardhat --network hardhat test test/contract/*.spec.4844.ts", 37: "test:compatibility": "yarn run build:0.6 && yarn run build:0.7", 38: "test:storage": "./test/storage/test.bash", 39: "test:signatures": "./test/signatures/test-sigs.bash", 40: "test:e2e": "hardhat test test/e2e/*.ts", 41: "postinstall": "patch-package", 42: "deploy-factory": "hardhat run scripts/deployment.ts", 43: "deploy-eth-rollup": "hardhat run scripts/createEthRollup.ts", 44: "deploy-erc20-rollup": "hardhat run scripts/createERC20Rollup.ts", 45: "script:bold-prepare": "hardhat run ./scripts/prepareBoldUpgrade.ts", 46: "script:bold-populate-lookup": "hardhat run ./scripts/populateLookup.ts", 47: "script:bold-local-execute": "hardhat run ./scripts/testLocalExecuteBoldUpgrade.ts", 48: "script:bold-verify": "hardhat run ./scripts/testVerifyBoldUpgrade.ts" 49: }, 50: "dependencies": { 51: "@offchainlabs/upgrade-executor": "1.1.0-beta.0", 52: "@openzeppelin/contracts": "4.7.3", 53: "@openzeppelin/contracts-upgradeable": "4.7.3", 54: "patch-package": "^6.4.7" 55: }, 56: "private": false, 57: "devDependencies": { 58: "@arbitrum/sdk": "^3.1.3", 59: "@ethersproject/providers": "^5.7.2", 60: "@nomiclabs/hardhat-ethers": "npm:hardhat-deploy-ethers@^0.3.0-beta.13", 61: "@nomiclabs/hardhat-etherscan": "^3.1.0", 62: "@nomiclabs/hardhat-waffle": "^2.0.1", 63: "@tovarishfin/hardhat-yul": "^3.0.5", 64: "@typechain/ethers-v5": "^10.0.0", 65: "@typechain/hardhat": "^6.0.0", 66: "@types/chai": "^4.3.0", 67: "@types/mocha": "^9.0.0", 68: "@types/node": "^17.0.5", 69: "@typescript-eslint/eslint-plugin": "^5.14.0", 70: "@typescript-eslint/eslint-plugin-tslint": "^5.27.1", 71: "@typescript-eslint/parser": "^5.14.0", 72: "chai": "^4.3.4", 73: "dotenv": "^16.3.1", 74: "eslint": "^8.23.1", 75: "eslint-config-prettier": "^8.3.0", 76: "eslint-plugin-mocha": "^9.0.0", 77: "eslint-plugin-prettier": "^4.0.0", 78: "ethereum-waffle": "^3.4.0", 79: "ethers": "^5.5.2", 80: "hardhat": "^2.17.2", 81: "hardhat-deploy": "^0.11.37", 82: "hardhat-gas-reporter": "^1.0.9", 83: "hardhat-ignore-warnings": "^0.2.9", 84: "hardhat-contract-sizer": "^2.10.0", 85: "postinstall-postinstall": "^2.1.0", 86: "prettier": "^2.5.1", 87: "prettier-plugin-solidity": "^1.0.0-beta.19", 88: "solhint": "^3.3.7", 89: "solhint-plugin-prettier": "^0.0.5", 90: "solidity-coverage": "^0.8.4", 91: "ts-node": "^10.4.0", 92: "tslint": "^6.1.3", 93: "typechain": "^8.0.0", 94: "typescript": "^4.5.4" 95: } 96: }
Setting boundaries on state variables in smart contracts is essential for maintaining system integrity and user protection. Without caps on values, variables could reach extremes that exploit or disrupt contract functionality, leading to potential loss or unintended consequences for users. Implementing checks for minimum and maximum permissible values can prevent such issues, ensuring variables remain within a safe and reasonable range. This practice guards against attacks aimed at destabilizing the contract, such as griefing, where attackers intentionally cause distress by exploiting vulnerabilities. Proper validation promotes contract reliability, user trust, and a healthier ecosystem by mitigating risks associated with unbounded state changes.
Num of instances: 2
['223']
223: function setBaseStake(uint256 newBaseStake) external override { 224: baseStake = newBaseStake; // <= FOUND 225: emit OwnerFunctionCalled(12); 226: }
['204']
</details> <details> <summary># For Information Only - NonCritical findings </summary>204: function setMinimumAssertionPeriod(uint256 newPeriod) external override { 205: minimumAssertionPeriod = newPeriod; // <= FOUND 206: emit OwnerFunctionCalled(8); 207: }
When state variables (uints) that can be set to their maximum value (type(uint256).max for example) are used in "greater than" comparisons, it introduces a risk of logic errors. If the state variable ever reaches this max value, any comparison expecting it to increment further will fail. This can halt or disrupt contract functionality. To avoid this, implement checks to ensure that the state variable doesn't exceed a certain threshold below the max value.
Num of instances: 5
['162']
162: function stakeOnNewAssertion(AssertionInputs calldata assertion, bytes32 expectedAssertionHash) 163: public 164: onlyValidator 165: whenNotPaused 166: { 167: 168: require( 169: expectedAssertionHash == bytes32(0) 170: || getAssertionStorage(expectedAssertionHash).status == AssertionStatus.NoAssertion, 171: "EXPECTED_ASSERTION_SEEN" 172: ); 173: 174: require(isStaked(msg.sender), "NOT_STAKED"); 175: 176: 177: 178: 179: 180: 181: require(amountStaked(msg.sender) >= assertion.beforeStateData.configData.requiredStake, "INSUFFICIENT_STAKE"); 182: 183: bytes32 prevAssertion = RollupLib.assertionHash( 184: assertion.beforeStateData.prevPrevAssertionHash, 185: assertion.beforeState, 186: assertion.beforeStateData.sequencerBatchAcc 187: ); 188: getAssertionStorage(prevAssertion).requireExists(); 189: 190: 191: 192: 193: bytes32 lastAssertion = latestStakedAssertion(msg.sender); 194: require( 195: lastAssertion == prevAssertion || getAssertionStorage(lastAssertion).firstChildBlock > 0, 196: "STAKED_ON_ANOTHER_BRANCH" 197: ); 198: 199: (bytes32 newAssertionHash, bool overflowAssertion) = 200: createNewAssertion(assertion, prevAssertion, expectedAssertionHash); 201: _stakerMap[msg.sender].latestStakedAssertion = newAssertionHash; 202: 203: if (!overflowAssertion) { 204: uint256 timeSincePrev = block.number - getAssertionStorage(prevAssertion).createdAtBlock; 205: 206: require(timeSincePrev >= minimumAssertionPeriod, "TIME_DELTA"); // <= FOUND 207: } 208: 209: if (!getAssertionStorage(newAssertionHash).isFirstChild) { 210: 211: 212: 213: 214: IERC20(stakeToken).safeTransfer(loserStakeEscrow, assertion.beforeStateData.configData.requiredStake); 215: } 216: }
['605']
605: function delayProofImpl(uint256 afterDelayedMessagesRead, DelayProof memory delayProof) 606: internal 607: { 608: 609: if (afterDelayedMessagesRead > totalDelayedMessagesRead) { // <= FOUND 610: if (buffer.isUpdatable()) { 611: 612: bytes32 delayedAcc = bridge.delayedInboxAccs(totalDelayedMessagesRead); 613: 614: if ( 615: !Messages.isValidDelayedAccPreimage( 616: delayedAcc, 617: delayProof.beforeDelayedAcc, 618: delayProof.delayedMessage 619: ) 620: ) { 621: revert InvalidDelayedAccPreimage(); 622: } 623: buffer.update(delayProof.delayedMessage.blockNumber); 624: } 625: } 626: }
['628']
628: function isDelayProofRequired(uint256 afterDelayedMessagesRead) internal view returns (bool) { 629: 630: 631: return 632: isDelayBufferable && 633: afterDelayedMessagesRead > totalDelayedMessagesRead && // <= FOUND 634: !buffer.isSynced(); 635: }
['791']
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: ) 805: { 806: if (afterDelayedMessagesRead < totalDelayedMessagesRead) revert DelayedBackwards(); 807: if (afterDelayedMessagesRead > bridge.delayedMessageCount()) revert DelayedTooFar(); 808: 809: (seqMessageIndex, beforeAcc, delayedAcc, acc) = bridge.enqueueSequencerMessage( 810: dataHash, 811: afterDelayedMessagesRead, 812: prevMessageCount, 813: newMessageCount 814: ); 815: 816: totalDelayedMessagesRead = afterDelayedMessagesRead; // <= FOUND 817: 818: if (calldataLengthPosted > 0 && !isUsingFeeToken) { 819: 820: submitBatchSpendingReport(dataHash, seqMessageIndex, block.basefee, 0); 821: } 822: }
['204']
</details>204: function setMinimumAssertionPeriod(uint256 newPeriod) external override { 205: minimumAssertionPeriod = newPeriod; // <= FOUND 206: emit OwnerFunctionCalled(8); 207: }
The initializer modifiers ensures two key aspects. A) Only an authorised account can initialize B) initialization can only be done once. Consider using such a modifier and in instances where initialization can be done more than once, the function name should be changed to reflect that.
Num of instances: 2
['177']
177: function initialize( // <= FOUND 178: IBridge bridge_, 179: ISequencerInbox.MaxTimeVariation calldata maxTimeVariation_, 180: BufferConfig memory bufferConfig_ 181: ) external onlyDelegated { 182: if (bridge != IBridge(address(0))) revert AlreadyInit(); 183: if (bridge_ == IBridge(address(0))) revert HadZeroInit(); 184: 185: 186: 187: bool actualIsUsingFeeToken = false; 188: try IERC20Bridge(address(bridge_)).nativeToken() returns (address feeToken) { 189: if (feeToken != address(0)) { 190: actualIsUsingFeeToken = true; 191: } 192: } catch {} 193: if (isUsingFeeToken != actualIsUsingFeeToken) { 194: revert NativeTokenMismatch(); 195: } 196: 197: bridge = bridge_; 198: rollup = bridge_.rollup(); 199: 200: _setMaxTimeVariation(maxTimeVariation_); 201: 202: if (isDelayBufferable) { 203: _setBufferConfig(bufferConfig_); 204: } 205: }
['27']
</details>27: function initialize(address _stakeToken) external view override onlyProxy { // <= FOUND 28: require(_stakeToken != address(0), "NEED_STAKE_TOKEN"); 29: }
The same library was imported twice, as such one instance should be safely removed
Num of instances: 1
['5']
</details>5: 9: pragma solidity ^0.8.0; 10: 11: import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; 12: 13: import {IRollupUser} from "./IRollupLogic.sol"; 14: import "../libraries/UUPSNotUpgradeable.sol"; 15: import "./RollupCore.sol"; 16: import "./IRollupLogic.sol"; 17: import {ETH_POS_BLOCK_TIME} from "../libraries/Constants.sol"; 18: 19: contract RollupUserLogic is RollupCore, UUPSNotUpgradeable, IRollupUser { 20: using AssertionNodeLib for AssertionNode; 21: using GlobalStateLib for GlobalState; 22: using SafeERC20 for IERC20; 23: 24: modifier onlyValidator() { 25: require(isValidator[msg.sender] || validatorWhitelistDisabled, "NOT_VALIDATOR"); 26: _; 27: } 28: 31: function initialize(address _stakeToken) external view override onlyProxy { 32: require(_stakeToken != address(0), "NEED_STAKE_TOKEN"); 33: } 34: 35: uint256 internal immutable deployTimeChainId = block.chainid; 36: 37: function _chainIdChanged() internal view returns (bool) { 38: return deployTimeChainId != block.chainid; 39: } 40: 53: uint256 public constant VALIDATOR_AFK_BLOCKS = 201600; 54: 55: function _validatorIsAfk() internal view returns (bool) { 56: AssertionNode memory latestConfirmedAssertion = getAssertionStorage(latestConfirmed()); 57: if (latestConfirmedAssertion.createdAtBlock == 0) return false; 58: 59: 60: if (latestConfirmedAssertion.firstChildBlock > 0) { 61: return latestConfirmedAssertion.firstChildBlock + VALIDATOR_AFK_BLOCKS < block.number; 62: } 63: return latestConfirmedAssertion.createdAtBlock + VALIDATOR_AFK_BLOCKS < block.number; 64: } 65: 66: function removeWhitelistAfterFork() external { 67: require(!validatorWhitelistDisabled, "WHITELIST_DISABLED"); 68: require(_chainIdChanged(), "CHAIN_ID_NOT_CHANGED"); 69: validatorWhitelistDisabled = true; 70: } 71: 75: function removeWhitelistAfterValidatorAfk() external { 76: require(!validatorWhitelistDisabled, "WHITELIST_DISABLED"); 77: require(_validatorIsAfk(), "VALIDATOR_NOT_AFK"); 78: validatorWhitelistDisabled = true; 79: } 80: 86: function confirmAssertion( 87: bytes32 assertionHash, 88: bytes32 prevAssertionHash, 89: AssertionState calldata confirmState, 90: bytes32 winningEdgeId, 91: ConfigData calldata prevConfig, 92: bytes32 inboxAcc 93: ) external onlyValidator whenNotPaused { 94: 95: 105: 106: AssertionNode storage assertion = getAssertionStorage(assertionHash); 107: 108: 109: AssertionNode storage prevAssertion = getAssertionStorage(prevAssertionHash); 110: RollupLib.validateConfigHash(prevConfig, prevAssertion.configHash); 111: 112: 113: require(block.number >= assertion.createdAtBlock + prevConfig.confirmPeriodBlocks, "BEFORE_DEADLINE"); 114: 115: 116: require(prevAssertionHash == latestConfirmed(), "PREV_NOT_LATEST_CONFIRMED"); 117: 118: if (prevAssertion.secondChildBlock > 0) { 119: 120: ChallengeEdge memory winningEdge = IEdgeChallengeManager(prevConfig.challengeManager).getEdge(winningEdgeId); 121: require(winningEdge.claimId == assertionHash, "NOT_WINNER"); 122: require(winningEdge.status == EdgeStatus.Confirmed, "EDGE_NOT_CONFIRMED"); 123: require(winningEdge.confirmedAtBlock != 0, "ZERO_CONFIRMED_AT_BLOCK"); 124: 125: 126: 127: require( 128: block.number >= winningEdge.confirmedAtBlock + challengeGracePeriodBlocks, 129: "CHALLENGE_GRACE_PERIOD_NOT_PASSED" 130: ); 131: } 132: 133: confirmAssertionInternal(assertionHash, prevAssertionHash, confirmState, inboxAcc); 134: } 135: 140: function _newStake(uint256 depositAmount, address withdrawalAddress) internal onlyValidator whenNotPaused { 141: 142: require(!isStaked(msg.sender), "ALREADY_STAKED"); 143: 144: createNewStake(msg.sender, depositAmount, withdrawalAddress); 145: } 146: 153: function computeAssertionHash(bytes32 prevAssertionHash, AssertionState calldata state, bytes32 inboxAcc) 154: external 155: pure 156: returns (bytes32) 157: { 158: return RollupLib.assertionHash(prevAssertionHash, state, inboxAcc); 159: } 160: 166: function stakeOnNewAssertion(AssertionInputs calldata assertion, bytes32 expectedAssertionHash) 167: public 168: onlyValidator 169: whenNotPaused 170: { 171: 172: require( 173: expectedAssertionHash == bytes32(0) 174: || getAssertionStorage(expectedAssertionHash).status == AssertionStatus.NoAssertion, 175: "EXPECTED_ASSERTION_SEEN" 176: ); 177: 178: require(isStaked(msg.sender), "NOT_STAKED"); 179: 180: 181: 182: 183: 184: 185: require(amountStaked(msg.sender) >= assertion.beforeStateData.configData.requiredStake, "INSUFFICIENT_STAKE"); 186: 187: bytes32 prevAssertion = RollupLib.assertionHash( 188: assertion.beforeStateData.prevPrevAssertionHash, 189: assertion.beforeState, 190: assertion.beforeStateData.sequencerBatchAcc 191: ); 192: getAssertionStorage(prevAssertion).requireExists(); 193: 194: 195: 196: 197: bytes32 lastAssertion = latestStakedAssertion(msg.sender); 198: require( 199: lastAssertion == prevAssertion || getAssertionStorage(lastAssertion).firstChildBlock > 0, 200: "STAKED_ON_ANOTHER_BRANCH" 201: ); 202: 203: (bytes32 newAssertionHash, bool overflowAssertion) = 204: createNewAssertion(assertion, prevAssertion, expectedAssertionHash); 205: _stakerMap[msg.sender].latestStakedAssertion = newAssertionHash; 206: 207: if (!overflowAssertion) { 208: uint256 timeSincePrev = block.number - getAssertionStorage(prevAssertion).createdAtBlock; 209: 210: require(timeSincePrev >= minimumAssertionPeriod, "TIME_DELTA"); 211: } 212: 213: if (!getAssertionStorage(newAssertionHash).isFirstChild) { 214: 215: 216: 217: 218: IERC20(stakeToken).safeTransfer(loserStakeEscrow, assertion.beforeStateData.configData.requiredStake); 219: } 220: } 221: 225: function returnOldDeposit() external override onlyValidator whenNotPaused { 226: requireInactiveStaker(msg.sender); 227: withdrawStaker(msg.sender); 228: } 229: 235: function _addToDeposit(address stakerAddress, uint256 depositAmount) internal onlyValidator whenNotPaused { 236: require(isStaked(stakerAddress), "NOT_STAKED"); 237: increaseStakeBy(stakerAddress, depositAmount); 238: } 239: 244: function reduceDeposit(uint256 target) external onlyValidator whenNotPaused { 245: requireInactiveStaker(msg.sender); 246: 247: reduceStakeTo(msg.sender, target); 248: } 249: 255: function fastConfirmAssertion( 256: bytes32 assertionHash, 257: bytes32 parentAssertionHash, 258: AssertionState calldata confirmState, 259: bytes32 inboxAcc 260: ) public whenNotPaused { 261: require(msg.sender == anyTrustFastConfirmer, "NOT_FAST_CONFIRMER"); 262: 263: confirmAssertionInternal(assertionHash, parentAssertionHash, confirmState, inboxAcc); 264: } 265: 276: function fastConfirmNewAssertion(AssertionInputs calldata assertion, bytes32 expectedAssertionHash) 277: external 278: whenNotPaused 279: { 280: 281: require(expectedAssertionHash != bytes32(0), "EXPECTED_ASSERTION_HASH"); 282: AssertionStatus status = getAssertionStorage(expectedAssertionHash).status; 283: 284: bytes32 prevAssertion = RollupLib.assertionHash( 285: assertion.beforeStateData.prevPrevAssertionHash, 286: assertion.beforeState, 287: assertion.beforeStateData.sequencerBatchAcc 288: ); 289: getAssertionStorage(prevAssertion).requireExists(); 290: 291: if (status == AssertionStatus.NoAssertion) { 292: 293: (bytes32 newAssertionHash,) = createNewAssertion(assertion, prevAssertion, expectedAssertionHash); 294: if (!getAssertionStorage(newAssertionHash).isFirstChild) { 295: 296: 297: 298: IERC20(stakeToken).safeTransfer(loserStakeEscrow, assertion.beforeStateData.configData.requiredStake); 299: } 300: } 301: 302: 303: fastConfirmAssertion( 304: expectedAssertionHash, 305: prevAssertion, 306: assertion.afterState, 307: bridge.sequencerInboxAccs(assertion.afterState.globalState.getInboxPosition() - 1) 308: ); 309: } 310: 311: function owner() external view returns (address) { 312: return _getAdmin(); 313: } 314: 319: function newStakeOnNewAssertion( 320: uint256 tokenAmount, 321: AssertionInputs calldata assertion, 322: bytes32 expectedAssertionHash 323: ) external { 324: newStakeOnNewAssertion(tokenAmount, assertion, expectedAssertionHash, msg.sender); 325: } 326: 334: function newStakeOnNewAssertion( 335: uint256 tokenAmount, 336: AssertionInputs calldata assertion, 337: bytes32 expectedAssertionHash, 338: address withdrawalAddress 339: ) public { 340: require(withdrawalAddress != address(0), "EMPTY_WITHDRAWAL_ADDRESS"); 341: _newStake(tokenAmount, withdrawalAddress); 342: stakeOnNewAssertion(assertion, expectedAssertionHash); 343: 344: receiveTokens(tokenAmount); 345: } 346: 352: function addToDeposit(address stakerAddress, uint256 tokenAmount) external onlyValidator whenNotPaused { 353: _addToDeposit(stakerAddress, tokenAmount); 354: 355: receiveTokens(tokenAmount); 356: } 357: 361: function withdrawStakerFunds() external override whenNotPaused returns (uint256) { 362: uint256 amount = withdrawFunds(msg.sender); 363: require(amount > 0, "NO_FUNDS_TO_WITHDRAW"); 364: 365: IERC20(stakeToken).safeTransfer(msg.sender, amount); 366: return amount; 367: } 368: 369: function receiveTokens(uint256 tokenAmount) private { 370: IERC20(stakeToken).safeTransferFrom(msg.sender, address(this), tokenAmount); 371: } 372: } 373:
Critical functions, especially those affecting protocol parameters or user funds, are potential points of failure or exploitation. To mitigate risks, incorporating a timelock on such functions can be beneficial. A timelock requires a waiting period between the time an action is initiated and when it's executed, giving stakeholders time to react, potentially vetoing malicious or erroneous changes. To implement, integrate a smart contract like OpenZeppelin's TimelockController or build a custom mechanism. This ensures governance decisions or administrative changes are transparent and allows for community or multi-signature interventions, enhancing protocol security and trustworthiness.
Num of instances: 3
['63']
63: function setTemplates( // <= FOUND 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
['894']
894: function setIsBatchPoster(address addr, bool isBatchPoster_) // <= FOUND 895: external 896: onlyRollupOwnerOrBatchPosterManager 897:
['933']
</details>933: function setIsSequencer(address addr, bool isSequencer_) // <= FOUND 934: external 935: onlyRollupOwnerOrBatchPosterManager 936:
Implementing a two-step procedure for updating protocol addresses adds an extra layer of security. In such a system, the first step initiates the change, and the second step, after a predefined delay, confirms and finalizes it. This delay allows stakeholders or monitoring tools to observe and react to unintended or malicious changes. If an unauthorized change is detected, corrective actions can be taken before the change is finalized. To achieve this, introduce a "proposed address" state variable and a "delay period". Upon an update request, set the "proposed address". After the delay, if not contested, the main protocol address can be updated.
Num of instances: 4
['894']
894: function setIsBatchPoster(address addr, bool isBatchPoster_) // <= FOUND 895: external 896: onlyRollupOwnerOrBatchPosterManager 897: { 898: isBatchPoster[addr] = isBatchPoster_; 899: emit OwnerFunctionCalled(1); 900: }
['933']
933: function setIsSequencer(address addr, bool isSequencer_) // <= FOUND 934: external 935: onlyRollupOwnerOrBatchPosterManager 936: { 937: isSequencer[addr] = isSequencer_; 938: emit OwnerFunctionCalled(4); 939: }
['942']
942: function setBatchPosterManager(address newBatchPosterManager) external onlyRollupOwner { // <= FOUND 943: batchPosterManager = newBatchPosterManager; 944: emit OwnerFunctionCalled(5); 945: }
['63']
</details>63: function setTemplates( // <= FOUND 64: BridgeCreator _bridgeCreator, 65: IOneStepProofEntry _osp, 66: IEdgeChallengeManager _challengeManagerLogic, 67: IRollupAdmin _rollupAdminLogic, 68: IRollupUser _rollupUserLogic, 69: IUpgradeExecutor _upgradeExecutorLogic, 70: address _validatorWalletCreator, // <= FOUND 71: DeployHelper _l2FactoriesDeployer 72: ) external onlyOwner { 73: bridgeCreator = _bridgeCreator; 74: osp = _osp; 75: challengeManagerTemplate = _challengeManagerLogic; 76: rollupAdminLogic = _rollupAdminLogic; 77: rollupUserLogic = _rollupUserLogic; 78: upgradeExecutorLogic = _upgradeExecutorLogic; 79: validatorWalletCreator = _validatorWalletCreator; 80: l2FactoriesDeployer = _l2FactoriesDeployer; 81: emit TemplatesUpdated(); 82: }
Num of instances: 4
['5']
5: pragma solidity ^0.8.4; // <= FOUND
['5']
5: pragma solidity ^0.8.17; // <= FOUND
['5']
5: pragma solidity ^0.8.0; // <= FOUND
['7']
</details>7: pragma solidity >=0.6.9 <0.9.0; // <= FOUND
Custom error codes should be used in place of strings for revert statements in Solidity contracts to enhance efficiency and reduce gas costs. String-based error messages consume more bytecode and storage, increasing the overall gas consumption during contract deployment and execution
Num of instances: 1
['287']
</details>287: 288: revert("Both y and z cannot be zero"); // <= FOUND
Functions in Solidity that accept array parameters should incorporate length checks as a security measure. This is to prevent potential overflow errors, unwanted gas consumption, and manipulation attempts. Without length checks, an attacker could pass excessively large arrays as input, causing excessive computation and potentially causing the function to exceed the block gas limit, leading to a denial-of-service. Additionally, unexpected array sizes could lead to logic errors within the function. As a resolution, always validate array length at the start of functions handling array inputs, ensuring it aligns with the expectations of the function logic. This makes the code more robust and predictable.
Num of instances: 3
['539']
539: function confirmEdgeByOneStepProof( 540: bytes32 edgeId, 541: OneStepData calldata oneStepData, 542: ConfigData calldata prevConfig, 543: bytes32[] calldata beforeHistoryInclusionProof, // <= FOUND 544: bytes32[] calldata afterHistoryInclusionProof // <= FOUND 545: ) public { 546: bytes32 prevAssertionHash = store.getPrevAssertionHash(edgeId); 547: 548: assertionChain.validateConfig(prevAssertionHash, prevConfig); 549: 550: ExecutionContext memory execCtx = ExecutionContext({ 551: maxInboxMessagesRead: prevConfig.nextInboxPosition, 552: bridge: assertionChain.bridge(), 553: initialWasmModuleRoot: prevConfig.wasmModuleRoot 554: }); 555: 556: store.confirmEdgeByOneStepProof( 557: edgeId, 558: oneStepProofEntry, 559: oneStepData, 560: execCtx, 561: beforeHistoryInclusionProof, 562: afterHistoryInclusionProof, 563: NUM_BIGSTEP_LEVEL, 564: LAYERZERO_BIGSTEPEDGE_HEIGHT, 565: LAYERZERO_SMALLSTEPEDGE_HEIGHT 566: ); 567: 568: emit EdgeConfirmedByOneStepProof(edgeId, store.edges[edgeId].mutualId()); 569: }
['238']
238: function appendLeaf(bytes32[] memory me, bytes32 leaf) internal pure returns (bytes32[] memory) { // <= FOUND 239: 240: 241: return appendCompleteSubTree(me, 0, keccak256(abi.encodePacked(leaf))); 242: }
['359']
</details>359: function verifyInclusionProof(bytes32 rootHash, bytes32 leaf, uint256 index, bytes32[] memory proof) // <= FOUND 360: internal 361: pure 362: { 363: bytes32 calculatedRoot = MerkleLib.calculateRoot(proof, index, keccak256(abi.encodePacked(leaf))); 364: require(rootHash == calculatedRoot, "Invalid inclusion proof"); 365: }
Such instances can be replaced with unnamed returns
Num of instances: 1
['14']
</details>14: function leastSignificantBit(uint256 x) internal pure returns (uint256 msb) { // <= FOUND 15: require(x > 0, "Zero has no significant bits"); 16: 17: 18: uint256 isolated = ((x - 1) & x) ^ x; 19: 20: 21: return mostSignificantBit(isolated); 22: }
Sensitive setter functions in smart contracts often alter critical state variables. Without events emitted in these functions, external observers or dApps cannot easily track or react to these state changes. Missing events can obscure contract activity, hampering transparency and making integration more challenging. To resolve this, incorporate appropriate event emissions within these functions. Events offer an efficient way to log crucial changes, aiding in real-time tracking and post-transaction verification.
Num of instances: 11
['662']
662: function setConfirmedRival(EdgeStore storage store, bytes32 edgeId) internal { // <= FOUND 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: }
['239']
239: function setChildren(ChallengeEdge storage edge, bytes32 lowerChildId, bytes32 upperChildId) internal { // <= FOUND 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']
249: function setConfirmed(ChallengeEdge storage edge) internal { // <= FOUND 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']
264: function setRefunded(ChallengeEdge storage edge) internal { // <= FOUND 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: }
['847']
847: function _setBufferConfig(BufferConfig memory bufferConfig_) internal { // <= FOUND 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: 862: if (bridge.delayedMessageCount() == totalDelayedMessagesRead) { 863: buffer.update(uint64(block.number)); 864: } 865: }
['867']
867: function _setMaxTimeVariation(ISequencerInbox.MaxTimeVariation memory maxTimeVariation_) // <= FOUND 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: }
['208']
208: function updateRollupAddress() external { // <= FOUND 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: }
['68']
68: function update(BufferData storage self, uint64 blockNumber) internal { // <= FOUND 69: self.bufferBlocks = calcPendingBuffer(self, blockNumber); 70: 71: 72: 73: self.prevBlockNumber = blockNumber; 74: self.prevSequencedBlockNumber = uint64(block.number); 75: }
['502']
502: function updateTimerCache(EdgeStore storage store, bytes32 edgeId, uint256 newValue) // <= FOUND 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: 509: if (newValue > currentAccuTimer) { 510: store.edges[edgeId].totalTimeUnrivaledCache = uint64(newValue); 511: return (true, newValue); 512: } 513: return (false, currentAccuTimer); 514: }
['516']
516: function updateTimerCacheByChildren(EdgeStore storage store, bytes32 edgeId) internal returns (bool, uint256) { // <= FOUND 517: return updateTimerCache(store, edgeId, timeUnrivaledTotal(store, edgeId)); 518: }
['520']
</details>520: function updateTimerCacheByClaim( // <= FOUND 521: EdgeStore storage store, 522: bytes32 edgeId, 523: bytes32 claimingEdgeId, 524: uint8 numBigStepLevel 525: ) internal returns (bool, uint256) { 526: 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: }
Function parameters in Solidity are passed by value, meaning they are essentially local copies. Mutating them can lead to confusion and errors because the changes don't persist outside the function. By keeping function parameters immutable, you ensure clarity in code behavior, preventing unintended side-effects. If you need to modify a value based on a parameter, use a local variable inside the function, leaving the original parameter unaltered. By adhering to this practice, you maintain a clear distinction between input data and the internal processing logic, improving code readability and reducing the potential for bugs.
Num of instances: 3
['502']
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: 509: if (newValue > currentAccuTimer) { 510: store.edges[edgeId].totalTimeUnrivaledCache = uint64(newValue); 511: return (true, newValue); 512: } 513: return (false, currentAccuTimer); 514: }
['755']
755: function submitBatchSpendingReport( 756: bytes32 dataHash, 757: uint256 seqMessageIndex, 758: uint256 gasPrice, 759: uint256 extraGas 760: ) internal { 761: 762: 763: 764: address batchPoster = tx.origin; 765: 766: 767: 768: if (hostChainIsArbitrum) { 769: 770: uint256 l1Fees = ArbGasInfo(address(0x6c)).getCurrentTxL1GasFees(); 771: extraGas += l1Fees / block.basefee; 772: } 773: if (extraGas > type(uint64).max) revert ExtraGasNotUint64(); 774: bytes memory spendingReportMsg = abi.encodePacked( 775: block.timestamp, 776: batchPoster, 777: dataHash, 778: seqMessageIndex, 779: gasPrice, 780: uint64(extraGas) 781: ); 782: 783: uint256 msgNum = bridge.submitBatchSpendingReport( 784: batchPoster, 785: keccak256(spendingReportMsg) 786: ); 787: 788: emit InboxMessageDelivered(msgNum, spendingReportMsg); 789: }
['33']
</details>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) { 42: uint256 elapsed = end > start ? end - start : 0; 43: uint256 delay = sequenced > start ? sequenced - start : 0; 44: 45: 46: buffer += (elapsed * replenishRateInBasis) / BASIS; 47: 48: uint256 unexpectedDelay = delay > threshold ? delay - threshold : 0; 49: if (unexpectedDelay > elapsed) { 50: unexpectedDelay = elapsed; 51: } 52: 53: 54: if (buffer > unexpectedDelay) { 55: buffer -= unexpectedDelay; 56: if (buffer > threshold) { 57: 58: return buffer > max ? max : buffer; 59: } 60: } 61: 62: return threshold; 63: }
Num of instances: 7
['24']
24: constructor(address _stakeToken) { 25: stakeToken = _stakeToken; // <= FOUND 26: }
['287']
287: constructor( 288: Contracts memory contracts, 289: ProxyAdmins memory proxyAdmins, 290: Implementations memory implementations, 291: Settings memory settings 292: ) { 293: L1_TIMELOCK = contracts.l1Timelock; 294: OLD_ROLLUP = contracts.rollup; 295: BRIDGE = contracts.bridge; 296: SEQ_INBOX = contracts.sequencerInbox; 297: REI = contracts.rollupEventInbox; 298: OUTBOX = contracts.outbox; 299: INBOX = contracts.inbox; 300: OSP = contracts.osp; 301: 302: PROXY_ADMIN_OUTBOX = ProxyAdmin(proxyAdmins.outbox); 303: PROXY_ADMIN_BRIDGE = ProxyAdmin(proxyAdmins.bridge); 304: PROXY_ADMIN_REI = ProxyAdmin(proxyAdmins.rei); 305: PROXY_ADMIN_SEQUENCER_INBOX = ProxyAdmin(proxyAdmins.seqInbox); 306: PROXY_ADMIN_INBOX = ProxyAdmin(proxyAdmins.inbox); 307: PREIMAGE_LOOKUP = new StateHashPreImageLookup(); 308: ROLLUP_READER = new RollupReader(contracts.rollup); 309: 310: IMPL_BRIDGE = implementations.bridge; 311: IMPL_SEQUENCER_INBOX = implementations.seqInbox; 312: IMPL_INBOX = implementations.inbox; 313: IMPL_REI = implementations.rei; 314: IMPL_OUTBOX = implementations.outbox; 315: IMPL_PATCHED_OLD_ROLLUP_USER = implementations.oldRollupUser; 316: IMPL_NEW_ROLLUP_USER = implementations.newRollupUser; 317: IMPL_NEW_ROLLUP_ADMIN = implementations.newRollupAdmin; 318: IMPL_CHALLENGE_MANAGER = implementations.challengeManager; 319: 320: CHAIN_ID = settings.chainId; 321: CONFIRM_PERIOD_BLOCKS = settings.confirmPeriodBlocks; 322: CHALLENGE_PERIOD_BLOCKS = settings.challengePeriodBlocks; 323: STAKE_TOKEN = settings.stakeToken; 324: STAKE_AMOUNT = settings.stakeAmt; 325: MINI_STAKE_AMOUNTS_STORAGE = address(new ConstantArrayStorage(settings.miniStakeAmounts)); 326: ANY_TRUST_FAST_CONFIRMER = settings.anyTrustFastConfirmer; 327: DISABLE_VALIDATOR_WHITELIST = settings.disableValidatorWhitelist; 328: BLOCK_LEAF_SIZE = settings.blockLeafSize; 329: BIGSTEP_LEAF_SIZE = settings.bigStepLeafSize; 330: SMALLSTEP_LEAF_SIZE = settings.smallStepLeafSize; 331: NUM_BIGSTEP_LEVEL = settings.numBigStepLevel; // <= FOUND 332: CHALLENGE_GRACE_PERIOD_BLOCKS = settings.challengeGracePeriodBlocks; 333: IS_DELAY_BUFFERABLE = settings.isDelayBufferable; 334: MAX = settings.bufferConfig.max; 335: THRESHOLD = settings.bufferConfig.threshold; 336: REPLENISH_RATE_IN_BASIS = settings.bufferConfig.replenishRateInBasis; 337: }
['31']
31: constructor( 32: address _rollup, 33: bytes32 _assertionHash 34: ) AbsBoldStakingPool(IRollupCore(_rollup).stakeToken()) { 35: rollup = _rollup; // <= FOUND 36: assertionHash = _assertionHash; 37: }
['126']
126: constructor(IOldRollup _rollup) { 127: rollup = _rollup; // <= FOUND 128: }
['45']
45: constructor( 46: BridgeTemplates memory _ethBasedTemplates, 47: BridgeTemplates memory _erc20BasedTemplates 48: ) Ownable() { 49: ethBasedTemplates = _ethBasedTemplates; // <= FOUND 50: erc20BasedTemplates = _erc20BasedTemplates; // <= FOUND 51: }
['169']
169: constructor(uint256[] memory __array) { 170: _array = __array; // <= FOUND 171: }
['35']
</details>35: constructor( 36: address _challengeManager, 37: bytes32 _edgeId 38: ) AbsBoldStakingPool(address(EdgeChallengeManager(_challengeManager).stakeToken())) { 39: challengeManager = _challengeManager; // <= FOUND 40: edgeId = _edgeId; 41: }
The definition of a constant or immutable variable is that they do not change, casting such variables can potentially push more than one 'value' to a constant, for example a uin128 constant can have its original value and that of when it's casted to uint64 (i.e it has some bytes truncated). This can create confusion and inconsistencies within the code which can inadvertently increase the attack surface of the project. It is thus advise to either change the uint byte size in the constant/immutable definition of the variable or introduce a second state variable to cover the differing casts that are expected such as having a uint128 constant and a separate uint64 constant.
Num of instances: 2
['46']
46: IERC20(stakeToken).safeIncreaseAllowance(address(challengeManager), requiredStake); // <= FOUND
['472']
</details>472: 473: IEdgeChallengeManager challengeManager = IEdgeChallengeManager( 474: address( 475: new TransparentUpgradeableProxy( 476: address(IMPL_CHALLENGE_MANAGER), // <= FOUND 477: address(PROXY_ADMIN_BRIDGE), 478: "" 479: ) 480: ) 481: );
Using transfer libraries like OpenZeppelin's Address.sendValue is preferred over low-level calls for transferring Ether in Solidity. These libraries provide clearer, more semantically meaningful methods compared to low-level call() functions. They encapsulate best practices for error handling and gas management, enhancing the security and readability of your code. Low-level calls lack these built-in safety checks and can be more error-prone, especially when dealing with Ether transfers.
Num of instances: 1
['304']
</details>304: 305: 306: (bool sent, ) = msg.sender.call{value: address(this).balance}(""); // <= FOUND
Only some address parameters are checked against address(0), to ensure consistency ensure all address parameters are checked.
Num of instances: 2
['287']
287: function _deployFactories( 288: address _inbox, 289: address _nativeToken, 290: uint256 _maxFeePerGas 291: ) internal { 292: if (_nativeToken == address(0)) { 293: 294: uint256 cost = l2FactoriesDeployer.getDeploymentTotalCost( 295: IInboxBase(_inbox), 296: _maxFeePerGas 297: ); 298: 299: 300: l2FactoriesDeployer.perform{value: cost}(_inbox, _nativeToken, _maxFeePerGas); 301: 302: 303: 304: (bool sent, ) = msg.sender.call{value: address(this).balance}(""); 305: require(sent, "Refund failed"); 306: } else { 307: 308: uint256 totalFee = l2FactoriesDeployer.getDeploymentTotalCost( 309: IInboxBase(_inbox), 310: _maxFeePerGas 311: ); 312: IERC20(_nativeToken).safeTransferFrom(msg.sender, _inbox, totalFee); 313: 314: 315: l2FactoriesDeployer.perform(_inbox, _nativeToken, _maxFeePerGas); 316: } 317: }
['99']
</details>99: function createBridge( 100: address adminProxy, 101: address rollup, 102: address nativeToken, 103: ISequencerInbox.MaxTimeVariation calldata maxTimeVariation, 104: BufferConfig calldata bufferConfig 105: ) external returns (BridgeContracts memory) { 106: 107: bool isDelayBufferable = bufferConfig.threshold != 0; 108: 109: 110: BridgeContracts memory frame = _createBridge( 111: adminProxy, 112: nativeToken == address(0) ? ethBasedTemplates : erc20BasedTemplates, 113: isDelayBufferable 114: ); 115: 116: 117: if (nativeToken == address(0)) { 118: IEthBridge(address(frame.bridge)).initialize(IOwnable(rollup)); 119: } else { 120: IERC20Bridge(address(frame.bridge)).initialize(IOwnable(rollup), nativeToken); 121: } 122: frame.sequencerInbox.initialize(IBridge(frame.bridge), maxTimeVariation, bufferConfig); 123: frame.inbox.initialize(frame.bridge, frame.sequencerInbox); 124: frame.rollupEventInbox.initialize(frame.bridge); 125: frame.outbox.initialize(frame.bridge); 126: 127: return frame; 128: }
Having such variables can create confusion in both developers and in users of the project. Consider renaming these variables to improve code clarity.
Num of instances: 20
['344']
344: uint64 stakerCount = ROLLUP_READER.stakerCount(); // <= FOUND
['219']
219: 227: event EdgeAdded( 228: bytes32 indexed edgeId, 229: bytes32 indexed mutualId, 230: bytes32 indexed originId, 231: bytes32 claimId, 232: uint256 length, // <= FOUND 233: uint8 level, 234: bool hasRival, // <= FOUND 235: bool isLayerZero 236: );
['20']
20: event RollupCreated( 21: address indexed rollupAddress, 22: address indexed nativeToken, 23: address inboxAddress, 24: address outbox, 25: address rollupEventInbox, 26: address challengeManager, 27: address adminProxy, 28: address sequencerInbox, // <= FOUND 29: address bridge, 30: address upgradeExecutor, 31: address validatorWalletCreator 32: );
['17']
17: uint256 amountStaked; // <= FOUND
['41']
41: uint256 amountStaked; // <= FOUND
['103']
103: uint256 length; // <= FOUND
['27']
27: 28: uint64 stakerCount; // <= FOUND
['22']
22: address owner; // <= FOUND
['280']
280: address sequencerInbox; // <= FOUND
['50']
50: address validatorWalletCreator; // <= FOUND
['105']
105: bool hasRival; // <= FOUND
['20']
20: bool isStaked; // <= FOUND
['46']
46: bool isStaked; // <= FOUND
['26']
26: 27: bool isFirstChild; // <= FOUND
['117']
117: 118: bool isPending; // <= FOUND
['106']
106: bool isLayerZero; // <= FOUND
['66']
66: error EdgeNotLengthOne(uint256 length); // <= FOUND
['19']
19: error NotOwner(address sender, address owner); // <= FOUND
['47']
47: error NotRollupOrOwner(address sender, address rollup, address owner); // <= FOUND
['128']
</details>128: 133: function withdrawableFunds(address owner) external view returns (uint256); // <= FOUND
Not casting an integer as a string before passing it into abi.encode can result in unintended behaviour. lets say '1' being encoded as '1' it will be encoded as char(1) which is the 'start of heading' control character or id of 60 would be encoded as '<'. This is may not be intended. To rectify this, simply cast the Id as a string with string(id) or ideally use solmate's libString library (toString)
Num of instances: 6
['637']
637: function packHeader(uint256 afterDelayedMessagesRead) 638: internal 639: view 640: returns (bytes memory, IBridge.TimeBounds memory) 641: { 642: IBridge.TimeBounds memory timeBounds = getTimeBounds(); 643: bytes memory header = abi.encodePacked( 644: timeBounds.minTimestamp, 645: timeBounds.maxTimestamp, 646: timeBounds.minBlockNumber, 647: timeBounds.maxBlockNumber, 648: uint64(afterDelayedMessagesRead) 649: ); 650: 651: assert(header.length == HEADER_LENGTH); 652: return (header, timeBounds); 653: }
['755']
755: function submitBatchSpendingReport( 756: bytes32 dataHash, 757: uint256 seqMessageIndex, 758: uint256 gasPrice, 759: uint256 extraGas 760: ) internal { 761: 762: 763: 764: address batchPoster = tx.origin; 765: 766: 767: 768: if (hostChainIsArbitrum) { 769: 770: uint256 l1Fees = ArbGasInfo(address(0x6c)).getCurrentTxL1GasFees(); 771: extraGas += l1Fees / block.basefee; 772: } 773: if (extraGas > type(uint64).max) revert ExtraGasNotUint64(); 774: bytes memory spendingReportMsg = abi.encodePacked( 775: block.timestamp, 776: batchPoster, 777: dataHash, 778: seqMessageIndex, 779: gasPrice, 780: uint64(extraGas) 781: ); 782: 783: uint256 msgNum = bridge.submitBatchSpendingReport( 784: batchPoster, 785: keccak256(spendingReportMsg) 786: ); 787: 788: emit InboxMessageDelivered(msgNum, spendingReportMsg); 789: }
['54']
54: function configHash( 55: bytes32 wasmModuleRoot, 56: uint256 requiredStake, 57: address challengeManager, 58: uint64 confirmPeriodBlocks, 59: uint64 nextInboxPosition 60: ) internal pure returns (bytes32) { 61: return 62: keccak256( 63: abi.encodePacked( 64: wasmModuleRoot, 65: requiredStake, 66: challengeManager, 67: confirmPeriodBlocks, 68: nextInboxPosition 69: ) 70: ); 71: }
['101']
101: function stateHash(ExecutionState calldata executionState, uint256 inboxMaxCount) public pure returns (bytes32) { // <= FOUND 102: return // <= FOUND 103: keccak256(abi.encodePacked(executionState.globalState.hash(), inboxMaxCount, executionState.machineStatus)); 104: }
['166']
166: function mutualIdComponent( 167: uint8 level, 168: bytes32 originId, 169: uint256 startHeight, 170: bytes32 startHistoryRoot, 171: uint256 endHeight 172: ) internal pure returns (bytes32) { 173: return keccak256(abi.encodePacked(level, originId, startHeight, startHistoryRoot, endHeight)); 174: }
['189']
</details>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) { 197: return keccak256( 198: abi.encodePacked( 199: mutualIdComponent(level, originId, startHeight, startHistoryRoot, endHeight), endHistoryRoot 200: ) 201: ); 202: }
Ensuring matching input array lengths in functions is crucial to prevent logical errors and inconsistencies in smart contracts. Mismatched array lengths can lead to incomplete operations or incorrect data handling, particularly if one array's length is assumed to reflect another's. For instance, iterating over a shorter array than intended could result in unprocessed elements from a longer array, potentially causing unintended consequences in contract execution. Validating that all input arrays have intended lengths before performing operations helps safeguard against such errors, ensuring that all elements are appropriately processed and the contract behaves as expected.
Num of instances: 5
['235']
235: for (uint256 i = 0; i < deployParams.validators.length; i++) { // <= FOUND 236: _vals[i] = true; // <= FOUND 237: }
['235']
235: for (uint256 i = 0; i < deployParams.validators.length; i++) { // <= FOUND 236: _vals[i] = true; // <= FOUND 237: }
['528']
528: for (uint256 i = 0; i < validators.length; i++) { // <= FOUND 529: require(ROLLUP_READER.isValidator(validators[i]), "UNEXPECTED_NEW_VALIDATOR"); 530: _vals[i] = true; // <= FOUND 531: }
['528']
528: for (uint256 i = 0; i < validators.length; i++) { // <= FOUND 529: require(ROLLUP_READER.isValidator(validators[i]), "UNEXPECTED_NEW_VALIDATOR"); 530: _vals[i] = true; // <= FOUND 531: }
['188']
</details>188: for (uint256 i = 0; i < me.length; i++) { // <= FOUND 189: 190: 191: 192: if (i < level) { 193: 194: 195: require(me[i] == 0, "Append above least significant bit"); 196: } else { 197: 198: if (accumHash == 0) { 199: 200: next[i] = me[i]; 201: } else { 202: 203: if (me[i] == 0) { 204: 205: next[i] = accumHash; // <= FOUND 206: 207: accumHash = 0; 208: } else { 209: 210: 211: 212: next[i] = 0; // <= FOUND 213: accumHash = keccak256(abi.encodePacked(me[i], accumHash)); 214: } 215: } 216: } 217: }
Avoiding arithmetic directly within array indices, like test[i + 2], is recommended to prevent errors such as index out of bounds or incorrect data access. This practice enhances code readability and maintainability. Instead, calculate the index beforehand, store it in a variable, and then use that variable to access the array element. This approach reduces mistakes, especially in complex loops or when handling dynamic data, ensuring that each access is clear and verifiable. It's about keeping code clean and understandable, minimizing potential bugs.
Num of instances: 5
['51']
51: full[arr1.length + i] = arr2[i]; // <= FOUND
['223']
223: next[next.length - 1] = accumHash; // <= FOUND
['228']
228: 229: 230: require(next[next.length - 1] != 0, "Last entry zero"); // <= FOUND
['18']
18: clone[clone.length - 1] = newItem; // <= FOUND
['37']
</details>37: newArr[i - startIndex] = arr[i]; // <= FOUND
In Solidity, using a single modifier to check msg.sender against two different addresses can make the code less flexible and harder to maintain. Instead, consider using multiple modifiers for greater control and clarity. Separating these checks into individual modifiers improves readability and reusability, as each modifier can then be applied independently to different functions according to specific access requirements. This approach enhances the modularity of the contract and makes it easier to update or extend access control logic in the future, aligning with best practices for smart contract development and security.
Num of instances: 1
['108']
</details> </details>108: modifier onlyRollupOwnerOrBatchPosterManager() { 109: if (msg.sender != rollup.owner() && msg.sender != batchPosterManager) { 110: revert NotBatchPosterManager(msg.sender); 111: } 112: _; 113: }
#0 - c4-judge
2024-06-05T11:45:06Z
Picodes marked the issue as grade-b
#1 - 0xSorryNotSorry
2024-06-07T10:48:19Z
Dear Sir @Picodes,
Thank you for the time to check on this.
With all due respect, I believe this report should be considered to deserve Grade-A when compared to the other A graded reports below which of their items are invalid, inflated(should be NC) or Out Of Scope
Low-1-2-3-6-12-13-20-23-24: Should be deemed as NC as they´re either comments or error messages
Low-4-8-24 : Covered in 4naly3er report
Low-11-14-16 : Invalid
Low-18-19-22: OOS as there is no Gas Pot
Non-Criticals - which are not subject to points as per new C4 rules - are submitted and included in the summary without labeling them as "Only Informational"
</details> <details><summary> For https://github.com/code-423n4/2024-05-arbitrum-foundation-findings/issues/66 </summary>L-1: Invalid
L-3: Invalid as this is enforced by the devs specifically: https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/challengeV2/EdgeChallengeManager.sol#L372-L373
L-4-17: Invalid as this is an admin function and should be treated as safe
L-5-9-15: Invalid as this is the intended logic.
L-6-8-12-19: Should be deemed as NC as these are based on comments
L-7-10-13-14-16-20-22: Invalid
L-21: OOS as per the contest readme. it´s always assumed that there will be an honest party
</details> <details><summary> For https://github.com/code-423n4/2024-05-arbitrum-foundation-findings/issues/65 </summary>L-2-3: Future Code Speculation which is deemed to be OOS as per the C4 Rubrick
L-4: Should be invalid since it's not the protocol's responsibility/interest
L-5: Should be deemed as NC as there´s no issue pointed out.
L-7: Invalid as Ethereum is the main point of calcs for everything, hence there can be no re-org issue
L-9: Invalid as required fees forwarded only
L-10: Invalid as this is enforced by the devs specifically: https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/challengeV2/EdgeChallengeManager.sol#L372-L373
L-11: Should be deemed as NC as pausing means a bold upcoming admin action like upgrading
</details> <details><summary> For https://github.com/code-423n4/2024-05-arbitrum-foundation-findings/issues/64 </summary>L-01: Invalid as the input is provided here by the admin: https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/rollup/BOLDUpgradeAction.sol#L464-L466
L-02: OOS as there is no Gas Pot
L-05-21: Invalid as these are admin functions and should be treated as safe
L-06: Invalid as the target is Eth
L-04 - 07 - 08 - 09 - 11 - 13 - 15 - 16 - 23 - 27 - 28: Covered in 4naly3er report
L-10-20-30: Should be deemed as NC
L-22 is duplicate of L-19
L-25 is almost duplicate of L-24
L-26: False Positive as per the context
</details>#2 - CrystallineButterfly
2024-06-07T11:17:02Z
Hello @0xSorryNotSorry
Respectfully I do not agree that the majority of my report (https://github.com//issues/68) is non critical. There's a lot more value in my suggestions than you are claiming. I claim the majority are useful, low and the grade A is valid. With more validity given the comparative reports. As I do not exaggerate my suggestions, or assert false potentials without sufficient proofs.
I of course have to defend my report, meanwhile; the other reports are good. And I just asked to see mine with more clarity.
#3 - c4-judge
2024-06-08T14:54:02Z
Picodes marked the issue as grade-a
#4 - Picodes
2024-06-08T14:57:40Z
@0xSorryNotSorry my main reason for giving grade-b to this report was that to me a lot of issues here are also NC (basically all issues suggesting to adding safety measures like address(0) checks, 2 step ownership, empty bytes, etc)
#5 - 0xSorryNotSorry
2024-06-08T18:59:18Z
Thank you for the consideration and for your time sir @Picodes,
I definitely understand your point. That's why I didn't try to invalidate fellow wardens' reports, with all respect, as they had the similar issues and sometimes more invalids then valids.