# # # # # # # # # # # # # # # # # # # # # #
# #
# |\_/| #
# | O O #
# | <> _ #
# | _/\------____ ((| |)) #
# | `--' | #
# ____|_ ___| |___.' #
# /_/_____/____/_______| #
# _ _ _ #
# | | | | | | #
# | |_| | ___ _ _ _ __ __| | #
# | _ | / _ \ | | | || '_ \ / _` | #
# | | | || (_) || |_| || | | || (_| | #
# \_| |_/ \___/ \__,_||_| |_| \__,_| #
# #
# # # # # # # # # # # # # # # # # # # # # #| Id | Title | Instances |
|---|---|---|
| [M-01] | Centralization issue caused by admin privileges | 2 |
Total: 2 instances over 1 issues.
| Id | Title | Instances |
|---|---|---|
| [L-01] | Unsafe downcast may overflow | 1 |
| [L-02] | Contracts can receive ETH but do not implement any function for withdrawal | 3 |
| [L-03] | assert is deprecated and it should not be used |
1 |
| [L-04] | Missing limits when setting min/max amounts | 1 |
| [L-05] | Missing contract-existence checks before low-level calls | 1 |
| [L-06] | External calls in an unbounded loop can result in a DoS | 1 |
| [L-07] | Lack of two-step update for critical addresses | 1 |
| [L-08] | Gas griefing/theft is possible on an unsafe external call | 2 |
| [L-09] | Unused/empty receive/fallback function |
2 |
| [L-10] | onlyOwner functions not accessible if owner renounces ownership |
1 |
| [L-11] | No access control on receive/payable fallback |
2 |
| [L-12] | Missing checks for address(0) |
1 |
Total: 17 instances over 12 issues.
| Id | Title | Instances |
|---|---|---|
| [NC-01] | Unused named return |
8 |
| [NC-02] | Variable initialization with default value | 12 |
| [NC-03] | Duplicated require/if statements should be refactored |
1 |
| [NC-04] | Inconsistent usage of require/error |
1 |
| [NC-05] | Unbounded loop may run out of gas | 12 |
| [NC-06] | Public functions not called internally | 8 |
| [NC-07] | Use of non-named numeric constants | 1 |
| [NC-08] | Constant names should be UPPERCASE | 1 |
| [NC-09] | Variable names don't follow the Solidity naming convention | 7 |
| [NC-10] | Use named mappings to improve code readability | 3 |
| [NC-11] | Event does not have proper indexed fields |
4 |
| [NC-12] | Use of constant variables instead of immutable |
5 |
| [NC-13] | Lack of specific import identifier |
8 |
| [NC-14] | Imports should be organized more systematically | 2 |
| [NC-15] | Parameter omission in events | 2 |
| [NC-16] | Consider disabling renounceOwnership |
1 |
| [NC-17] | Custom error without details |
22 |
| [NC-18] | Constants in comparisons should appear on the left side | 8 |
| [NC-19] | Layout order does not comply with best practices | 4 |
| [NC-20] | Function visibility order does not comply with best practices | 4 |
| [NC-21] | Long functions should be refactored into multiple functions | 2 |
| [NC-22] | Lines are too long | 1 |
| [NC-23] | Some variables have a implicit default visibility | 6 |
| [NC-24] | Contracts should have full test coverage | 1 |
| [NC-25] | Large or complicated code bases should implement invariant tests | 1 |
| [NC-26] | Inconsistent spacing in comments | 9 |
| [NC-27] | Assembly code should have explicit comments | 1 |
| [NC-28] | Use @inheritdoc rather than using a non-standard annotation |
4 |
| [NC-29] | Missing NatSpec @param |
24 |
| [NC-30] | Incomplete NatSpec @return |
14 |
Total: 178 instances over 30 issues.
| Id | Title | Instances | Gas Saved |
|---|---|---|---|
| [GAS-01] | Use custom errors instead of require/assert |
9 | 261 |
| [GAS-02] | State variables access within a loop | 4 | 1,060 |
| [GAS-03] | Cache state variables with stack variables | 2 | 1,200 |
| [GAS-04] | Avoid updating storage when the value hasn't changed | 3 | 2,100 |
| [GAS-05] | Use of emit inside a loop |
3 | 3,078 |
| [GAS-06] | Shortcircuit rules can be be used to optimize some gas usage | 1 | 2,100 |
| [GAS-07] | Cache multiple accesses of a mapping/array | 1 | 42 |
| [GAS-08] | Using private for constants saves gas |
8 | 67,248 |
| [GAS-09] | Using bool for storage incurs overhead |
1 | 17,100 |
| [GAS-10] | Functions that revert when called by normal users can be payable |
7 | 147 |
| [GAS-11] | Array length is not cached |
11 | 33 |
| [GAS-12] | Empty blocks should be removed or emit something | 2 | 8,012 |
| [GAS-13] | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead |
18 | 108 |
| [GAS-14] | Using pre instead of post increments/decrements | 8 | 40 |
| [GAS-15] | Avoid using _msgSender if not supporting EIP-2771 |
1 | 16 |
| [GAS-16] | Low level call can be optimized with assembly |
1 | 248 |
| [GAS-17] | Use assembly to emit an event |
13 | 494 |
| [GAS-18] | >=/<= costs less gas than >/< |
23 | 69 |
| [GAS-19] | internal functions only called once can be inlined to save gas |
1 | 20 |
| [GAS-20] | Consider activating via-ir for deploying |
1 | 250 |
| [GAS-21] | Unused named return variables without optimizer waste gas | 8 | 27 |
| [GAS-22] | Using this.<fn>() wastes gas |
3 | 300 |
| [GAS-23] | Function names can be optimized | 4 | 88 |
| [GAS-24] | Bytes constants are more efficient than string constants | 1 | 378 |
| [GAS-25] | Constructors can be marked payable |
3 | 63 |
| [GAS-26] | Long revert strings | 6 | 108 |
| [GAS-27] | Lack of unchecked in loops |
12 | 720 |
| [GAS-28] | uint comparison with zero can be cheaper |
1 | 4 |
| [GAS-29] | Use assembly to check for address(0) |
1 | 6 |
Total: 157 instances over 29 issues with an estimate of 105,320 gas saved.
The owner role has a single point of failure and onlyOwner can use critical functions, posing a centralization issue. There is always a chance for owner keys to be stolen, and in such a case, the attacker can cause serious damage to the project due to important functions.
There are 2 instances of this issue.
File: src/ARMProxy.sol
27: function setARM(address arm) public onlyOwner {[27]
File: src/ManyChainMultiSig.sol
392: function setConfig([392]
When a type is downcast to a smaller type, the higher order bits are discarded, resulting in the application of a modulo operation to the original value.
If the downcasted value is large enough, this may result in an overflow that will not revert.
There is 1 instance of this issue.
File: src/ManyChainMultiSig.sol
// @audit uint256 i downcasted to uint8
464: Signer({addr: signerAddresses[i], index: uint8(i), group: signerGroups[i]});[464]
The following contracts can receive ETH, but can not withdraw: ETH is may be sent by users by mistake, and it will be stuck in those contracts.
There are 3 instances of this issue.
File: src/CallProxy.sol
16: fallback() external payable {[16]
File: src/ManyChainMultiSig.sol
45: receive() external payable {}[45]
File: src/RBACTimelock.sol
183: receive() external payable {}[183]
Prior to Solidity 0.8.0, the big difference between the assert and require is that the former uses up all the remaining gas and reverts all the changes made when the predicate is false.
It should be avoided even after Solidity version 0.8.0 as the documentation states:
Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix.
There is 1 instance of this issue.
File: src/ManyChainMultiSig.sol
452: assert(s_config.signers.length == 0);[452]
There are some missing limits in these functions, and this could lead to unexpected scenarios. Consider adding a min/max limit for the following values, when appropriate.
There is 1 instance of this issue.
File: src/RBACTimelock.sol
// @audit missing both checks -> newDelay
360: function updateDelay(uint256 newDelay) external virtual onlyRole(ADMIN_ROLE) {[360]
Low-level calls return success if there is no code present at the specified address, and this could lead to unexpected scenarios.
Ensure that the code is initialized by checking <address>.code.length > 0.
There is 1 instance of this issue.
File: src/ManyChainMultiSig.sol
489: (bool success, bytes memory ret) = target.call{value: value}(data);[489]
Consider limiting the number of iterations in loops that make external calls, as just a single one of them failing will result in a revert.
There is 1 instance of this issue.
File: src/ManyChainMultiSig.sol
// @audit recover (219)
217: for (uint256 i = 0; i < signatures.length; i++) {[217]
Add a two-step process for any critical address changes.
There is 1 instance of this issue.
File: src/ARMProxy.sol
27: function setARM(address arm) public onlyOwner {
28: if (arm == address(0)) revert ZeroAddressNotAllowed();
29: s_arm = arm;
30: emit ARMSet(arm);
31: }[27-31]
A low-level call will copy any amount of bytes to local memory. When bytes are copied from returndata to memory, the memory expansion cost is paid.
This means that when using a standard solidity call, the callee can 'returnbomb' the caller, imposing an arbitrary gas cost.
Because this gas is paid by the caller and in the caller's context, it can cause the caller to run out of gas and halt execution.
Consider replacing all unsafe call with excessivelySafeCall from this repository.
There are 2 instances of this issue.
File: src/ManyChainMultiSig.sol
489: (bool success, bytes memory ret) = target.call{value: value}(data);[489]
File: src/RBACTimelock.sol
331: (bool success, ) = call.target.call{value: call.value}(call.data);[331]
If the intention is for the ETH to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth)))
There are 2 instances of this issue.
File: src/ManyChainMultiSig.sol
45: receive() external payable {}[45]
File: src/RBACTimelock.sol
183: receive() external payable {}[183]
The owner is able to perform certain privileged activities, but it's possible to set the owner to address(0). This can represent a certain risk if the ownership is renounced for any other reason than by design.
Renouncing ownership will leave the contract without an owner, therefore limiting any functionality that needs authority.
There is 1 instance of this issue.
File: src/ManyChainMultiSig.sol
398: ) external onlyOwner {[398]
Users may send ETH by mistake to these contracts. As there is no access control on these functions, the funds will be permanently lost.
There are 2 instances of this issue.
File: src/ManyChainMultiSig.sol
45: receive() external payable {}[45]
File: src/RBACTimelock.sol
183: receive() external payable {}[183]
Check for zero-address to avoid the risk of setting address(0) for state variables.
There is 1 instance of this issue.
File: src/CallProxy.sol
12: i_target = target;[12]
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 8 instances of this issue.
File: src/ManyChainMultiSig.sol
507: function getRoot() public view returns (bytes32 root, uint32 validUntil) {
509: return (currentRootAndOpCount.root, currentRootAndOpCount.validUntil);[507]
File: src/RBACTimelock.sol
196: function isOperation(bytes32 id) public view virtual returns (bool registered) {
197: return getTimestamp(id) > 0;
203: function isOperationPending(bytes32 id) public view virtual returns (bool pending) {
204: return getTimestamp(id) > _DONE_TIMESTAMP;
210: function isOperationReady(bytes32 id) public view virtual returns (bool ready) {
212: return timestamp > _DONE_TIMESTAMP && timestamp <= block.timestamp;
218: function isOperationDone(bytes32 id) public view virtual returns (bool done) {
219: return getTimestamp(id) == _DONE_TIMESTAMP;
226: function getTimestamp(bytes32 id) public view virtual returns (uint256 timestamp) {
227: return _timestamps[id];
235: function getMinDelay() public view virtual returns (uint256 duration) {
236: return _minDelay;
247: ) public pure virtual returns (bytes32 hash) {
248: return keccak256(abi.encode(calls, predecessor, salt));[196, 203, 210, 218, 226, 235, 247]
It's not necessary to initialize a variable with its default value, and it's actually worse in gas terms as it adds an overhead.
There are 12 instances of this issue.
File: src/ManyChainMultiSig.sol
217: for (uint256 i = 0; i < signatures.length; i++) {
412: for (uint256 i = 0; i < signerGroups.length; i++) {
420: for (uint256 j = 0; j < NUM_GROUPS; j++) {
445: for (uint256 i = 0; i < oldSigners.length; i++) {
459: for (uint256 i = 0; i < signerAddresses.length; i++) {File: src/RBACTimelock.sol
145: for (uint256 i = 0; i < proposers.length; ++i) {
150: for (uint256 i = 0; i < executors.length; ++i) {
155: for (uint256 i = 0; i < cancellers.length; ++i) {
160: for (uint256 i = 0; i < bypassers.length; ++i) {
269: for (uint256 i = 0; i < calls.length; ++i) {
318: for (uint256 i = 0; i < calls.length; ++i) {
474: for (uint256 i = 0; i < calls.length; ++i) {[145, 150, 155, 160, 269, 318, 474]
These statements should be refactored to a separate function, as there are multiple parts of the codebase that use the same logic, to improve the code readability and reduce code duplication.
There is 1 instance of this issue.
File: src/RBACTimelock.sol
// @audit this require is duplicated on line 347
339: require(isOperationReady(id), "RBACTimelock: operation is not ready");[339]
Some parts of the codebase use require statements, while others use custom errors. Consider refactoring the code to use the same approach: the following findings represent the minority of require vs error, and they show the first occurance in each file, for brevity.
There is 1 instance of this issue.
File: src/RBACTimelock.sol
279: require(!isOperation(id), "RBACTimelock: operation already scheduled");[279]
Consider limiting the number of iterations in loops with an explicit revert reason to improve the user experience.
The function would eventually revert if out of gas anyway, but by limiting it the user avoids wasting too much gas, as the loop doesn't execute if an excessive value is provided.
There are 12 instances of this issue.
Expand findings
File: src/ManyChainMultiSig.sol
217: for (uint256 i = 0; i < signatures.length; i++) {
218: Signature calldata sig = signatures[i];
219: address signerAddress = ECDSA.recover(signedHash, sig.v, sig.r, sig.s);
220: // the off-chain system is required to sort the signatures by the
221: // signer address in an increasing order
222: if (prevAddress >= signerAddress) {
223: revert SignersAddressesMustBeStrictlyIncreasing();
224: }
225: prevAddress = signerAddress;
226:
227: signer = s_signers[signerAddress];
228: if (signer.addr != signerAddress) {
229: revert InvalidSigner();
230: }
231: uint8 group = signer.group;
232: while (true) {
233: groupVoteCounts[group]++;
234: if (groupVoteCounts[group] != s_config.groupQuorums[group]) {
235: // bail out unless we just hit the quorum. we only hit each quorum once,
236: // so we never move on to the parent of a group more than once.
237: break;
238: }
239: if (group == 0) {
240: // reached root
241: break;
242: }
243:
244: group = s_config.groupParents[group];
245: }
246: }
412: for (uint256 i = 0; i < signerGroups.length; i++) {
413: if (signerGroups[i] >= NUM_GROUPS) {
414: revert OutOfBoundsGroup();
415: }
416: groupChildrenCounts[signerGroups[i]]++;
417: }
420: for (uint256 j = 0; j < NUM_GROUPS; j++) {
421: uint256 i = NUM_GROUPS - 1 - j;
422: // ensure we have a well-formed group tree. the root should have itself as parent
423: if ((i != 0 && groupParents[i] >= i) || (i == 0 && groupParents[i] != 0)) {
424: revert GroupTreeNotWellFormed();
425: }
426: bool disabled = groupQuorums[i] == 0;
427: if (disabled) {
428: // a disabled group shouldn't have any children
429: if (0 < groupChildrenCounts[i]) {
430: revert SignerInDisabledGroup();
431: }
432: } else {
433: // ensure that the group quorum can be met
434: if (groupChildrenCounts[i] < groupQuorums[i]) {
435: revert OutOfBoundsGroupQuorum();
436: }
437: groupChildrenCounts[groupParents[i]]++;
438: // the above line clobbers groupChildrenCounts[0] in last iteration, don't use it after the loop ends
439: }
440: }
445: for (uint256 i = 0; i < oldSigners.length; i++) {
446: address oldSignerAddress = oldSigners[i].addr;
447: delete s_signers[oldSignerAddress];
448: s_config.signers.pop();
449: }
459: for (uint256 i = 0; i < signerAddresses.length; i++) {
460: if (prevSigner >= signerAddresses[i]) {
461: revert SignersAddressesMustBeStrictlyIncreasing();
462: }
463: Signer memory signer =
464: Signer({addr: signerAddresses[i], index: uint8(i), group: signerGroups[i]});
465: s_signers[signerAddresses[i]] = signer;
466: s_config.signers.push(signer);
467: prevSigner = signerAddresses[i];
468: }[217-246, 412-417, 420-440, 445-449, 459-468]
File: src/RBACTimelock.sol
145: for (uint256 i = 0; i < proposers.length; ++i) {
146: _setupRole(PROPOSER_ROLE, proposers[i]);
147: }
150: for (uint256 i = 0; i < executors.length; ++i) {
151: _setupRole(EXECUTOR_ROLE, executors[i]);
152: }
155: for (uint256 i = 0; i < cancellers.length; ++i) {
156: _setupRole(CANCELLER_ROLE, cancellers[i]);
157: }
160: for (uint256 i = 0; i < bypassers.length; ++i) {
161: _setupRole(BYPASSER_ROLE, bypassers[i]);
162: }
269: for (uint256 i = 0; i < calls.length; ++i) {
270: _checkFunctionSelectorNotBlocked(calls[i].data);
271: emit CallScheduled(id, i, calls[i].target, calls[i].value, calls[i].data, predecessor, salt, delay);
272: }
318: for (uint256 i = 0; i < calls.length; ++i) {
319: _execute(calls[i]);
320: emit CallExecuted(id, i, calls[i].target, calls[i].value, calls[i].data);
321: }
474: for (uint256 i = 0; i < calls.length; ++i) {
475: _execute(calls[i]);
476: emit BypasserCallExecuted(i, calls[i].target, calls[i].value, calls[i].data);
477: }[145-147, 150-152, 155-157, 160-162, 269-272, 318-321, 474-477]
Those functions should be declared as external instead of public, as they are never called internally by the contract.
Contracts are allowed to override their parents' functions and change the visibility from external to public.
There are 8 instances of this issue.
File: src/ManyChainMultiSig.sol
499: function getConfig() public view returns (Config memory) {
503: function getOpCount() public view returns (uint40) {
507: function getRoot() public view returns (bytes32 root, uint32 validUntil) {
512: function getRootMetadata() public view returns (RootMetadata memory) {File: src/RBACTimelock.sol
261: function scheduleBatch(
291: function cancel(bytes32 id) public virtual onlyRoleOrAdminRole(CANCELLER_ROLE) {
310: function executeBatch(
471: function bypasserExecuteBatch(Constants should be defined instead of using magic numbers.
There is 1 instance of this issue.
File: src/RBACTimelock.sol
485: if (data.length < 4) {[485]
Constants should be named with all capital letters with underscores separating words. Examples: MAX_BLOCKS, TOKEN_NAME. Documentation
There is 1 instance of this issue.
File: src/ARMProxy.sol
16: string public constant override typeAndVersion = "ARMProxy 1.0.0";[16]
Use mixedCase for local and state variables that are not constants, and add a trailing underscore for internal variables. Documentation
There are 7 instances of this issue.
File: src/ARMProxy.sol
19: address private s_arm;[19]
File: src/CallProxy.sol
9: address immutable i_target;[9]
File: src/ManyChainMultiSig.sol
58: mapping(address => Signer) s_signers;
121: Config s_config;
124: mapping(bytes32 => bool) s_seenSignedHashes;
143: ExpiringRootAndOpCount s_expiringRootAndOpCount;
171: RootMetadata s_rootMetadata;Since solidity 0.8.18 or later it's possible to use named mappings to make it easier to understand the purpose of each mapping. Documentation
There are 3 instances of this issue.
File: src/ManyChainMultiSig.sol
58: mapping(address => Signer) s_signers;
124: mapping(bytes32 => bool) s_seenSignedHashes;File: src/RBACTimelock.sol
66: mapping(bytes32 => uint256) private _timestamps;[66]
Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields).
Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
There are 4 instances of this issue.
File: src/ARMProxy.sol
12: event ARMSet(address arm);[12]
File: src/CallProxy.sol
7: event TargetSet(address target);[7]
File: src/ManyChainMultiSig.sol
524: event ConfigSet(Config config, bool isRootCleared);[524]
File: src/RBACTimelock.sol
102: event MinDelayChange(uint256 oldDuration, uint256 newDuration);[102]
Constant variables and immutable variables serve different purposes and should be used accordingly.
To clarify, constant variables are used for literal values in the code, whereas immutable variables are ideal for values calculated or passed into the constructor.
There are 5 instances of this issue.
File: src/RBACTimelock.sol
59: bytes32 public constant ADMIN_ROLE = keccak256("ADMIN_ROLE");
60: bytes32 public constant PROPOSER_ROLE = keccak256("PROPOSER_ROLE");
61: bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE");
62: bytes32 public constant CANCELLER_ROLE = keccak256("CANCELLER_ROLE");
63: bytes32 public constant BYPASSER_ROLE = keccak256("BYPASSER_ROLE");It is better to use import {<identifier>} from "<file.sol>" instead of import "<file.sol>" to improve readability and speed up the compilation time.
There are 8 instances of this issue.
File: src/ManyChainMultiSig.sol
4: import "openzeppelin-contracts/utils/cryptography/MerkleProof.sol";
5: import "openzeppelin-contracts/access/Ownable2Step.sol";
6: import "openzeppelin-contracts/utils/cryptography/ECDSA.sol";File: src/RBACTimelock.sol
4: import "openzeppelin-contracts/access/AccessControlEnumerable.sol";
5: import "openzeppelin-contracts/token/ERC721/IERC721Receiver.sol";
6: import "openzeppelin-contracts/token/ERC1155/IERC1155Receiver.sol";
7: import "openzeppelin-contracts/utils/Address.sol";
8: import "openzeppelin-contracts/utils/structs/EnumerableSet.sol";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.
There are 2 instances of this issue.
File: src/ARMProxy.sol
7: import {OwnerIsCreator} from "./OwnerIsCreator.sol";[7]
File: src/ManyChainMultiSig.sol
5: import "openzeppelin-contracts/access/Ownable2Step.sol";[5]
Events are generally emitted when sensitive changes are made to the contracts.
However, some are missing important parameters, as they should include both the new value and old value where possible.
There are 2 instances of this issue.
File: src/ARMProxy.sol
30: emit ARMSet(arm);[30]
File: src/ManyChainMultiSig.sol
305: emit NewRoot(root, validUntil, metadata);[305]
If it is not within the scope of the project to ultimately relinquish all ownership control, it is recommended to override the renounceOwnership function in OpenZeppelin's Ownable contract in order to disable it.
There is 1 instance of this issue.
File: src/ManyChainMultiSig.sol
44: contract ManyChainMultiSig is Ownable2Step {[44]
Consider adding some parameters to the error to indicate which user or values caused the failure.
There are 22 instances of this issue.
File: src/ARMProxy.sol
10: error ZeroAddressNotAllowed();[10]
File: src/ManyChainMultiSig.sol
530: error OutOfBoundsNumOfSigners();
533: error SignerGroupsLengthMismatch();
536: error OutOfBoundsGroup();
539: error GroupTreeNotWellFormed();
542: error OutOfBoundsGroupQuorum();
545: error SignerInDisabledGroup();
549: error SignersAddressesMustBeStrictlyIncreasing();
552: error InvalidSigner();
556: error InsufficientSigners();
559: error WrongChainId();
563: error WrongMultiSig();
566: error WrongPostOpCount();
570: error PendingOps();
573: error WrongPreOpCount();
576: error ProofCannotBeVerified();
580: error RootExpired();
584: error WrongNonce();
588: error PostOpCountReached();
594: error ValidUntilHasAlreadyPassed();
597: error MissingConfig();
600: error SignedHashAlreadySeen();[530, 533, 536, 539, 542, 545, 549, 552, 556, 559, 563, 566, 570, 573, 576, 580, 584, 588, 594, 597, 600]
This is useful to avoid doing some typo bugs.
There are 8 instances of this issue.
File: src/ManyChainMultiSig.sol
239: if (group == 0) {
249: if (s_config.groupQuorums[0] == 0) {
399: if (signerAddresses.length == 0 || signerAddresses.length > MAX_NUM_SIGNERS) {
423: if ((i != 0 && groupParents[i] >= i) || (i == 0 && groupParents[i] != 0)) {
423: if ((i != 0 && groupParents[i] >= i) || (i == 0 && groupParents[i] != 0)) {
423: if ((i != 0 && groupParents[i] >= i) || (i == 0 && groupParents[i] != 0)) {
426: bool disabled = groupQuorums[i] == 0;
452: assert(s_config.signers.length == 0);[239, 249, 399, 423, 423, 423, 426, 452]
This is a best practice that should be followed.
Inside each contract, library or interface, use the following order:
- Type declarations
- State variables
- Events
- Errors
- Modifiers
- Functions
There are 4 instances of this issue.
File: src/ARMProxy.sol
// @audit error ZeroAddressNotAllowed found on line 10
12: event ARMSet(address arm);
// @audit error ZeroAddressNotAllowed found on line 10
16: string public constant override typeAndVersion = "ARMProxy 1.0.0";[12]
File: src/CallProxy.sol
// @audit event TargetSet found on line 7
9: address immutable i_target;[9]
File: src/ManyChainMultiSig.sol
// @audit function found on line 45
47: uint8 public constant NUM_GROUPS = 32;
// @audit function getRootMetadata found on line 512
521: event NewRoot(bytes32 indexed root, uint32 validUntil, RootMetadata metadata);
// @audit function getRootMetadata found on line 512
530: error OutOfBoundsNumOfSigners();[47]
File: src/RBACTimelock.sol
// @audit function constructor found on line 128
172: modifier onlyRoleOrAdminRole(bytes32 role) {[172]
This is a best practice that should be followed.
Functions should be grouped according to their visibility and ordered:
- constructor
- receive function (if exists)
- fallback function (if exists)
- external
- public
- internal
- private
Within a grouping, place the view and pure functions last.
There are 4 instances of this issue.
File: src/ARMProxy.sol
// @audit setARM on line 27, which is public
44: fallback() external {[44]
File: src/ManyChainMultiSig.sol
// @audit _execute on line 488, which is internal
512: function getRootMetadata() public view returns (RootMetadata memory) {[512]
File: src/RBACTimelock.sol
// @audit _checkFunctionSelectorNotBlocked on line 484, which is private
328: function _execute(
// @audit _checkFunctionSelectorNotBlocked on line 484, which is private
454: function getBlockedFunctionSelectorAt(uint256 index) external view returns (bytes4) {Consider splitting long functions into multiple, smaller functions to improve the code readability.
There are 2 instances of this issue.
File: src/ManyChainMultiSig.sol
194: function setRoot(
392: function setConfig(Maximum suggested line length is 120 characters according to the documentation.
There is 1 instance of this issue.
File: src/RBACTimelock.sol
188: function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, AccessControlEnumerable) returns (bool) {[188]
Consider always adding an explicit visibility modifier for variables, as the default is internal.
There are 6 instances of this issue.
File: src/CallProxy.sol
9: address immutable i_target;[9]
File: src/ManyChainMultiSig.sol
58: mapping(address => Signer) s_signers;
121: Config s_config;
124: mapping(bytes32 => bool) s_seenSignedHashes;
143: ExpiringRootAndOpCount s_expiringRootAndOpCount;
171: RootMetadata s_rootMetadata;A 100% test coverage is not foolproof, but it helps immensely in reducing the amount of bugs that may occur.
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.
Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file.
There are 9 instances of this issue.
File: src/ARMProxy.sol
15: // solhint-disable-next-line chainlink-solidity/all-caps-constant-storage-variables
42: // their input/output behaviour should be identical to calling the proxied s_armFile: src/ManyChainMultiSig.sol
31: /// be used any more. Second, we embed metadata in the tree itself that has to be proven/revealed
61: // Signers are the leaves of the tree. A signer/leaf node is successful iff it furnishes a valid
62: // signature. A group/interior node is successful iff a quorum of its children are successful.
114: // groups form a tree structure (where the root/0-th signer group points to itself as
150: /// openzeppelin-contracts/contracts/utils/cryptography/MerkleProof.sol:15 for details.
155: // https://ethereum-magicians.org/t/eip-2294-explicit-bound-to-chain-id/11090) to
312: /// openzeppelin-contracts/contracts/utils/cryptography/MerkleProof.sol:15 for details.[31, 61, 62, 114, 150, 155, 312]
Low-level languages like assembly should require extensive documentation and comments to clarify the purpose of each instruction.
There is 1 instance of this issue.
File: src/CallProxy.sol
18: assembly {[18]
Consider using @inheritdoc instead of a custom annotation.
There are 4 instances of this issue.
File: src/RBACTimelock.sol
185: /**
186: * @dev See {IERC165-supportsInterface}.
187: */
188: function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, AccessControlEnumerable) returns (bool) {
365: /**
366: * @dev See {IERC721Receiver-onERC721Received}.
367: */
368: function onERC721Received(
377: /**
378: * @dev See {IERC1155Receiver-onERC1155Received}.
379: */
380: function onERC1155Received(
390: /**
391: * @dev See {IERC1155Receiver-onERC1155BatchReceived}.
392: */
393: function onERC1155BatchReceived([185-188, 365-368, 377-380, 390-393]
Some functions have an incomplete NatSpec: add a @param notation to describe the function parameters to improve the code documentation.
There are 24 instances of this issue.
Expand findings
File: src/ManyChainMultiSig.sol
// @audit missing target, value, data
486: /// @notice Execute an op's call. Performs a raw call that always succeeds if the
487: /// target isn't a contract.
488: function _execute(address target, uint256 value, bytes calldata data) internal virtual {[486-488]
File: src/RBACTimelock.sol
// @audit missing interfaceId
185: /**
186: * @dev See {IERC165-supportsInterface}.
187: */
188: function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, AccessControlEnumerable) returns (bool) {
// @audit missing id
192: /**
193: * @dev Returns whether an id correspond to a registered operation. This
194: * includes both Pending, Ready and Done operations.
195: */
196: function isOperation(bytes32 id) public view virtual returns (bool registered) {
// @audit missing id
200: /**
201: * @dev Returns whether an operation is pending or not.
202: */
203: function isOperationPending(bytes32 id) public view virtual returns (bool pending) {
// @audit missing id
207: /**
208: * @dev Returns whether an operation is ready or not.
209: */
210: function isOperationReady(bytes32 id) public view virtual returns (bool ready) {
// @audit missing id
215: /**
216: * @dev Returns whether an operation is done or not.
217: */
218: function isOperationDone(bytes32 id) public view virtual returns (bool done) {
// @audit missing id
222: /**
223: * @dev Returns the timestamp at with an operation becomes ready (0 for
224: * unset operations, 1 for done operations).
225: */
226: function getTimestamp(bytes32 id) public view virtual returns (uint256 timestamp) {
// @audit missing calls, predecessor, salt
239: /**
240: * @dev Returns the identifier of an operation containing a batch of
241: * transactions.
242: */
243: function hashOperationBatch(
// @audit missing calls, predecessor, salt, delay
251: /**
252: * @dev Schedule an operation containing a batch of transactions.
253: *
254: * Emits one {CallScheduled} event per transaction in the batch.
255: *
256: * Requirements:
257: *
258: * - the caller must have the 'proposer' or 'admin' role.
259: * - all payloads must not start with a blocked function selector.
260: */
261: function scheduleBatch(
// @audit missing id, delay
275: /**
276: * @dev Schedule an operation that is to becomes valid after a given delay.
277: */
278: function _schedule(bytes32 id, uint256 delay) private {
// @audit missing id
284: /**
285: * @dev Cancel an operation.
286: *
287: * Requirements:
288: *
289: * - the caller must have the 'canceller' or 'admin' role.
290: */
291: function cancel(bytes32 id) public virtual onlyRoleOrAdminRole(CANCELLER_ROLE) {
// @audit missing calls, predecessor, salt
298: /**
299: * @dev Execute an (ready) operation containing a batch of transactions.
300: * Note that we perform a raw call to each target. Raw calls to targets that
301: * don't have associated contract code will always succeed regardless of
302: * payload.
303: *
304: * Emits one {CallExecuted} event per transaction in the batch.
305: *
306: * Requirements:
307: *
308: * - the caller must have the 'executor' or 'admin' role.
309: */
310: function executeBatch(
// @audit missing call
325: /**
326: * @dev Execute an operation's call.
327: */
328: function _execute(
// @audit missing id, predecessor
335: /**
336: * @dev Checks before execution of an operation's calls.
337: */
338: function _beforeCall(bytes32 id, bytes32 predecessor) private view {
// @audit missing id
343: /**
344: * @dev Checks after execution of an operation's calls.
345: */
346: function _afterCall(bytes32 id) private {
// @audit missing newDelay
351: /**
352: * @dev Changes the minimum timelock duration for future operations.
353: *
354: * Emits a {MinDelayChange} event.
355: *
356: * Requirements:
357: *
358: * - the caller must have the 'admin' role.
359: */
360: function updateDelay(uint256 newDelay) external virtual onlyRole(ADMIN_ROLE) {
365: /**
366: * @dev See {IERC721Receiver-onERC721Received}.
367: */
368: function onERC721Received(
377: /**
378: * @dev See {IERC1155Receiver-onERC1155Received}.
379: */
380: function onERC1155Received(
390: /**
391: * @dev See {IERC1155Receiver-onERC1155BatchReceived}.
392: */
393: function onERC1155BatchReceived(
// @audit missing selector
403: /*
404: * New functions not present in original OpenZeppelin TimelockController
405: */
406:
407: /**
408: * @dev Blocks a function selector from being used, i.e. schedule
409: * operations with this function selector will revert.
410: *
411: * Requirements:
412: *
413: * - the caller must have the 'admin' role.
414: */
415: function blockFunctionSelector(bytes4 selector) external onlyRole(ADMIN_ROLE) {
// @audit missing selector
421: /**
422: * @dev Unblocks a previously blocked function selector so it can be used again.
423: * Requirements:
424: *
425: * - the caller must have the 'admin' role.
426: */
427: function unblockFunctionSelector(bytes4 selector) external onlyRole(ADMIN_ROLE) {
// @audit missing index
440: /**
441: * @dev Returns the blocked function selector with the given index. Function
442: * selectors are not sorted in any particular way, and their ordering may
443: * change at any point.
444: *
445: * WARNING: When using {getBlockedFunctionSelectorCount} and
446: * {getBlockedFunctionSelectorAt} via RPC, make sure you perform all queries
447: * on the same block. When using these functions within an onchain
448: * transaction, make sure that the state of this contract hasn't changed in
449: * between invocations to avoid time-of-check time-of-use bugs.
450: * See the following
451: * https://forum.openzeppelin.com/t/iterating-over-elements-on-enumerableset-in-openzeppelin-contracts/2296[forum
452: * post] for more information.
453: */
454: function getBlockedFunctionSelectorAt(uint256 index) external view returns (bytes4) {
// @audit missing calls
458: /**
459: * @dev Directly execute a batch of transactions, bypassing any other
460: * checks.
461: * Note that we perform a raw call to each target. Raw calls to targets that
462: * don't have associated contract code will always succeed regardless of
463: * payload.
464: *
465: * Emits one {BypasserCallExecuted} event per transaction in the batch.
466: *
467: * Requirements:
468: *
469: * - the caller must have the 'bypasser' or 'admin' role.
470: */
471: function bypasserExecuteBatch(
// @audit missing data
480: /**
481: * @dev Checks to see if the function being scheduled is blocked. This
482: * is used when trying to schedule or batch schedule an operation.
483: */
484: function _checkFunctionSelectorNotBlocked(bytes calldata data) private view {[185-188, 192-196, 200-203, 207-210, 215-218, 222-226, 239-243, 251-261, 275-278, 284-291, 298-310, 325-328, 335-338, 343-346, 351-360, 365-368, 377-380, 390-393, 403-415, 421-427, 440-454, 458-471, 480-484]
Some functions have an incomplete NatSpec: add a @return notation to describe the function return value to improve the code documentation.
There are 14 instances of this issue.
Expand findings
File: src/ManyChainMultiSig.sol
// @audit missing @return
495: /*
496: * Getters
497: */
498:
499: function getConfig() public view returns (Config memory) {[495-499]
File: src/RBACTimelock.sol
// @audit missing @return
185: /**
186: * @dev See {IERC165-supportsInterface}.
187: */
188: function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, AccessControlEnumerable) returns (bool) {
// @audit missing @return
192: /**
193: * @dev Returns whether an id correspond to a registered operation. This
194: * includes both Pending, Ready and Done operations.
195: */
196: function isOperation(bytes32 id) public view virtual returns (bool registered) {
// @audit missing @return
200: /**
201: * @dev Returns whether an operation is pending or not.
202: */
203: function isOperationPending(bytes32 id) public view virtual returns (bool pending) {
// @audit missing @return
207: /**
208: * @dev Returns whether an operation is ready or not.
209: */
210: function isOperationReady(bytes32 id) public view virtual returns (bool ready) {
// @audit missing @return
215: /**
216: * @dev Returns whether an operation is done or not.
217: */
218: function isOperationDone(bytes32 id) public view virtual returns (bool done) {
// @audit missing @return
222: /**
223: * @dev Returns the timestamp at with an operation becomes ready (0 for
224: * unset operations, 1 for done operations).
225: */
226: function getTimestamp(bytes32 id) public view virtual returns (uint256 timestamp) {
// @audit missing @return
230: /**
231: * @dev Returns the minimum delay for an operation to become valid.
232: *
233: * This value can be changed by executing an operation that calls `updateDelay`.
234: */
235: function getMinDelay() public view virtual returns (uint256 duration) {
// @audit missing @return
239: /**
240: * @dev Returns the identifier of an operation containing a batch of
241: * transactions.
242: */
243: function hashOperationBatch(
// @audit missing @return
365: /**
366: * @dev See {IERC721Receiver-onERC721Received}.
367: */
368: function onERC721Received(
// @audit missing @return
377: /**
378: * @dev See {IERC1155Receiver-onERC1155Received}.
379: */
380: function onERC1155Received(
// @audit missing @return
390: /**
391: * @dev See {IERC1155Receiver-onERC1155BatchReceived}.
392: */
393: function onERC1155BatchReceived(
// @audit missing @return
433: /**
434: * @dev Returns the number of blocked function selectors.
435: */
436: function getBlockedFunctionSelectorCount() external view returns (uint256) {
// @audit missing @return
440: /**
441: * @dev Returns the blocked function selector with the given index. Function
442: * selectors are not sorted in any particular way, and their ordering may
443: * change at any point.
444: *
445: * WARNING: When using {getBlockedFunctionSelectorCount} and
446: * {getBlockedFunctionSelectorAt} via RPC, make sure you perform all queries
447: * on the same block. When using these functions within an onchain
448: * transaction, make sure that the state of this contract hasn't changed in
449: * between invocations to avoid time-of-check time-of-use bugs.
450: * See the following
451: * https://forum.openzeppelin.com/t/iterating-over-elements-on-enumerableset-in-openzeppelin-contracts/2296[forum
452: * post] for more information.
453: */
454: function getBlockedFunctionSelectorAt(uint256 index) external view returns (bytes4) {[185-188, 192-196, 200-203, 207-210, 215-218, 222-226, 230-235, 239-243, 365-368, 377-380, 390-393, 433-436, 440-454]
Consider the use of a custom error, as it leads to a cheaper deploy cost and run time cost. The run time cost is only relevant when the revert condition is met.
There are 9 instances of this issue.
File: src/ManyChainMultiSig.sol
452: assert(s_config.signers.length == 0);[452]
File: src/RBACTimelock.sol
279: require(!isOperation(id), "RBACTimelock: operation already scheduled");
280: require(delay >= getMinDelay(), "RBACTimelock: insufficient delay");
292: require(isOperationPending(id), "RBACTimelock: operation cannot be cancelled");
332: require(success, "RBACTimelock: underlying transaction reverted");
339: require(isOperationReady(id), "RBACTimelock: operation is not ready");
340: require(predecessor == bytes32(0) || isOperationDone(predecessor), "RBACTimelock: missing dependency");
347: require(isOperationReady(id), "RBACTimelock: operation is not ready");
489: require(!_blockedFunctionSelectors.contains(bytes32(selector)), "RBACTimelock: selector is blocked");[279, 280, 292, 332, 339, 340, 347, 489]
State variable reads and writes are more expensive than local variable reads and writes. Therefore, it is recommended to replace state variable reads and writes within loops with a local variable. Gas savings should be multiplied by the average loop length.
There are 4 instances of this issue.
File: src/ManyChainMultiSig.sol
// @audit s_config
234: if (groupVoteCounts[group] != s_config.groupQuorums[group]) {
// @audit s_config
244: group = s_config.groupParents[group];
// @audit s_config
448: s_config.signers.pop();
// @audit s_config
466: s_config.signers.push(signer);Caching of a state variable replaces each Gwarmaccess (100 gas) with a cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There are 2 instances of this issue.
File: src/ManyChainMultiSig.sol
// @audit s_config on lines 234, 244, 249
253: if (groupVoteCounts[0] < s_config.groupQuorums[0]) {
// @audit s_config on lines 443, 448, 452, 453, 454, 483
466: s_config.signers.push(signer);If the old value is equal to the new value, not re-storing the value will avoid a Gsreset (2900 gas), potentially at the expense of a Gcoldsload (2100 gas) or a Gwarmaccess (100 gas)
There are 3 instances of this issue.
File: src/ManyChainMultiSig.sol
// @audit s_expiringRootAndOpCount, s_rootMetadata
194: function setRoot(
// @audit s_expiringRootAndOpCount, s_rootMetadata
392: function setConfig(File: src/RBACTimelock.sol
// @audit _minDelay
360: function updateDelay(uint256 newDelay) external virtual onlyRole(ADMIN_ROLE) {[360]
Emitting an event inside a loop performs a LOG op N times, where N is the loop length. Consider refactoring the code to emit the event only once at the end of loop. Gas savings should be multiplied by the average loop length.
There are 3 instances of this issue.
File: src/RBACTimelock.sol
271: emit CallScheduled(id, i, calls[i].target, calls[i].value, calls[i].data, predecessor, salt, delay);
320: emit CallExecuted(id, i, calls[i].target, calls[i].value, calls[i].data);
476: emit BypasserCallExecuted(i, calls[i].target, calls[i].value, calls[i].data);Some conditions may be reordered to save an SLOAD (2100 gas), as we avoid reading state variables when the first part of the condition fails (with &&), or succeeds (with ||).
There are 1 instance of this issue.
File: src/ManyChainMultiSig.sol
// @audit switch with this condition
// !metadata.overridePreviousRoot && opCount != s_rootMetadata.postOpCount
283: if (opCount != s_rootMetadata.postOpCount && !metadata.overridePreviousRoot) {[283]
Consider using a local storage or calldata variable when accessing a mapping/array value multiple times.
This can be useful to avoid recalculating the mapping hash and/or the array offsets.
There is 1 instance of this issue.
File: src/ManyChainMultiSig.sol
// @audit s_config.groupParents on line 234
244: group = s_config.groupParents[group];[244]
Saves deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table.
There are 8 instances of this issue.
File: src/ARMProxy.sol
16: string public constant override typeAndVersion = "ARMProxy 1.0.0";[16]
File: src/ManyChainMultiSig.sol
47: uint8 public constant NUM_GROUPS = 32;
48: uint8 public constant MAX_NUM_SIGNERS = 200;File: src/RBACTimelock.sol
59: bytes32 public constant ADMIN_ROLE = keccak256("ADMIN_ROLE");
60: bytes32 public constant PROPOSER_ROLE = keccak256("PROPOSER_ROLE");
61: bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE");
62: bytes32 public constant CANCELLER_ROLE = keccak256("CANCELLER_ROLE");
63: bytes32 public constant BYPASSER_ROLE = keccak256("BYPASSER_ROLE");Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past. See source.
There is 1 instance of this issue.
File: src/ManyChainMultiSig.sol
124: mapping(bytes32 => bool) s_seenSignedHashes;[124]
If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function.
Marking the function as payable will lower the gas for legitimate callers, as the compiler will not include checks for whether a payment was provided.
The extra opcodes avoided are:
CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2)
which cost an average of about 21 gas per call to the function, in addition to the extra deployment cost.
There are 7 instances of this issue.
File: src/ARMProxy.sol
27: function setARM(address arm) public onlyOwner {[27]
File: src/ManyChainMultiSig.sol
392: function setConfig(
393: address[] calldata signerAddresses,
394: uint8[] calldata signerGroups,
395: uint8[NUM_GROUPS] calldata groupQuorums,
396: uint8[NUM_GROUPS] calldata groupParents,
397: bool clearRoot
398: ) external onlyOwner {[392-398]
File: src/RBACTimelock.sol
261: function scheduleBatch(
262: Call[] calldata calls,
263: bytes32 predecessor,
264: bytes32 salt,
265: uint256 delay
266: ) public virtual onlyRoleOrAdminRole(PROPOSER_ROLE) {
291: function cancel(bytes32 id) public virtual onlyRoleOrAdminRole(CANCELLER_ROLE) {
360: function updateDelay(uint256 newDelay) external virtual onlyRole(ADMIN_ROLE) {
415: function blockFunctionSelector(bytes4 selector) external onlyRole(ADMIN_ROLE) {
427: function unblockFunctionSelector(bytes4 selector) external onlyRole(ADMIN_ROLE) {Solidity compiler reads array length every iteration if not cached. Storage array requires an extra sload operation (100 gas), memory array requires an extra mload operation (3 gas).
There are 11 instances of this issue.
File: src/ManyChainMultiSig.sol
217: for (uint256 i = 0; i < signatures.length; i++) {
412: for (uint256 i = 0; i < signerGroups.length; i++) {
445: for (uint256 i = 0; i < oldSigners.length; i++) {
459: for (uint256 i = 0; i < signerAddresses.length; i++) {File: src/RBACTimelock.sol
145: for (uint256 i = 0; i < proposers.length; ++i) {
150: for (uint256 i = 0; i < executors.length; ++i) {
155: for (uint256 i = 0; i < cancellers.length; ++i) {
160: for (uint256 i = 0; i < bypassers.length; ++i) {
269: for (uint256 i = 0; i < calls.length; ++i) {
318: for (uint256 i = 0; i < calls.length; ++i) {
474: for (uint256 i = 0; i < calls.length; ++i) {[145, 150, 155, 160, 269, 318, 474]
Some functions don't have a body: consider commenting why, or add some logic. Otherwise, refactor the code and remove these functions.
There are 2 instances of this issue.
File: src/ManyChainMultiSig.sol
45: receive() external payable {}[45]
File: src/RBACTimelock.sol
183: receive() external payable {}[183]
Citing the documentation:
When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher.This is because the EVM operates on 32 bytes at a time.Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
For example, each operation involving a uint8 costs an extra ** 22 - 28 gas ** (depending on whether the other operand is also a variable of type uint8) as compared to ones involvinguint256, due to the compiler having to clear the higher bits of the memory word before operating on theuint8, as well as the associated stack operations of doing so.
Consider using a larger size, then downcast where needed.
There are 18 instances of this issue.
File: src/ManyChainMultiSig.sol
47: uint8 public constant NUM_GROUPS = 32;
48: uint8 public constant MAX_NUM_SIGNERS = 200;
52: uint8 index; // index of signer in s_config.signers
53: uint8 group; // 0 <= group < NUM_GROUPS. Each signer can only be in one group.
138: uint32 validUntil;
140: uint40 opCount;
160: uint40 preOpCount;
162: uint40 postOpCount;
175: uint8 v;
196: uint32 validUntil,
231: uint8 group = signer.group;
279: uint40 opCount = s_expiringRootAndOpCount.opCount;
316: uint40 nonce;
472: uint40 opCount = s_expiringRootAndOpCount.opCount;
503: function getOpCount() public view returns (uint40) {
507: function getRoot() public view returns (bytes32 root, uint32 validUntil) {
521: event NewRoot(bytes32 indexed root, uint32 validUntil, RootMetadata metadata);
527: event OpExecuted(uint40 indexed nonce, address to, bytes data, uint256 value);[47, 48, 52, 53, 138, 140, 160, 162, 175, 196, 231, 279, 316, 472, 503, 507, 521, 527]
Pre increments/decrements (++i/--i) are cheaper than post increments/decrements (i++/i--): it saves 6 gas per expression.
There are 8 instances of this issue.
File: src/ManyChainMultiSig.sol
217: for (uint256 i = 0; i < signatures.length; i++) {
233: groupVoteCounts[group]++;
412: for (uint256 i = 0; i < signerGroups.length; i++) {
416: groupChildrenCounts[signerGroups[i]]++;
420: for (uint256 j = 0; j < NUM_GROUPS; j++) {
437: groupChildrenCounts[groupParents[i]]++;
445: for (uint256 i = 0; i < oldSigners.length; i++) {
459: for (uint256 i = 0; i < signerAddresses.length; i++) {[217, 233, 412, 416, 420, 437, 445, 459]
Consider using using msg.sender directly when the contract does not implement EIP-2771, as it's cheaper.
There is 1 instance of this issue.
File: src/RBACTimelock.sol
173: address sender = _msgSender();[173]
returnData is copied to memory even if the variable is not utilized: the proper way to handle this is through a low level assembly call.```solidity
// before
(bool success,) = payable(receiver).call{gas: gas, value: value}("");
//after bool success; assembly { success := call(gas, receiver, value, 0, 0, 0, 0) }
*There is 1 instance of this issue.*
```solidity
File: src/RBACTimelock.sol
331: (bool success, ) = call.target.call{value: call.value}(call.data);
[331]
To efficiently emit events, it's possible to utilize assembly by making use of scratch space and the free memory pointer. This approach has the advantage of potentially avoiding the costs associated with memory expansion.
However, it's important to note that in order to safely optimize this process, it is preferable to cache and restore the free memory pointer.
A good example of such practice can be seen in Solady's codebase.
There are 13 instances of this issue.
File: src/ARMProxy.sol
30: emit ARMSet(arm);[30]
File: src/CallProxy.sol
13: emit TargetSet(target);[13]
File: src/ManyChainMultiSig.sol
305: emit NewRoot(root, validUntil, metadata);
372: emit OpExecuted(op.nonce, op.to, op.data, op.value);
483: emit ConfigSet(s_config, clearRoot);File: src/RBACTimelock.sol
165: emit MinDelayChange(0, minDelay);
271: emit CallScheduled(id, i, calls[i].target, calls[i].value, calls[i].data, predecessor, salt, delay);
295: emit Cancelled(id);
320: emit CallExecuted(id, i, calls[i].target, calls[i].value, calls[i].data);
361: emit MinDelayChange(_minDelay, newDelay);
417: emit FunctionSelectorBlocked(selector);
429: emit FunctionSelectorUnblocked(selector);
476: emit BypasserCallExecuted(i, calls[i].target, calls[i].value, calls[i].data);[165, 271, 295, 320, 361, 417, 429, 476]
The compiler uses opcodes GT and ISZERO for code that uses >, but only requires LT for >=. A similar behaviour applies for >, which uses opcodes LT and ISZERO, but only requires GT for <=.
There are 23 instances of this issue.
File: src/ManyChainMultiSig.sol
217: for (uint256 i = 0; i < signatures.length; i++) {
253: if (groupVoteCounts[0] < s_config.groupQuorums[0]) {
258: if (validUntil < block.timestamp) {
293: if (metadata.preOpCount > metadata.postOpCount) {
354: if (block.timestamp > currentExpiringRootAndOpCount.validUntil) {
399: if (signerAddresses.length == 0 || signerAddresses.length > MAX_NUM_SIGNERS) {
412: for (uint256 i = 0; i < signerGroups.length; i++) {
420: for (uint256 j = 0; j < NUM_GROUPS; j++) {
429: if (0 < groupChildrenCounts[i]) {
434: if (groupChildrenCounts[i] < groupQuorums[i]) {
445: for (uint256 i = 0; i < oldSigners.length; i++) {
459: for (uint256 i = 0; i < signerAddresses.length; i++) {[217, 253, 258, 293, 354, 399, 412, 420, 429, 434, 445, 459]
File: src/RBACTimelock.sol
145: for (uint256 i = 0; i < proposers.length; ++i) {
150: for (uint256 i = 0; i < executors.length; ++i) {
155: for (uint256 i = 0; i < cancellers.length; ++i) {
160: for (uint256 i = 0; i < bypassers.length; ++i) {
197: return getTimestamp(id) > 0;
204: return getTimestamp(id) > _DONE_TIMESTAMP;
212: return timestamp > _DONE_TIMESTAMP && timestamp <= block.timestamp;
269: for (uint256 i = 0; i < calls.length; ++i) {
318: for (uint256 i = 0; i < calls.length; ++i) {
474: for (uint256 i = 0; i < calls.length; ++i) {
485: if (data.length < 4) {[145, 150, 155, 160, 197, 204, 212, 269, 318, 474, 485]
Consider removing those internal functions and to put the logic directly where they are called, as they are called only once.
There is 1 instance of this issue.
File: src/ManyChainMultiSig.sol
488: function _execute(address target, uint256 value, bytes calldata data) internal virtual {[488]
The IR-based code generator was developed to make code generation more transparent and auditable, while also enabling powerful optimization passes that can be applied across functions.
It is possible to activate the IR-based code generator through the command line by using the flag --via-ir or by including the option {"viaIR": true}.
Keep in mind that compiling with this option may take longer. However, you can simply test it before deploying your code. If you find that it provides better performance, you can add the --via-ir flag to your deploy command.
Consider changing the variable to be an unnamed one, since the variable is never assigned, nor is it returned by name. If the optimizer is not turned on, leaving the code as it is will also waste gas for the stack variable.
There are 8 instances of this issue.
File: src/ManyChainMultiSig.sol
507: function getRoot() public view returns (bytes32 root, uint32 validUntil) {[507]
File: src/RBACTimelock.sol
196: function isOperation(bytes32 id) public view virtual returns (bool registered) {
203: function isOperationPending(bytes32 id) public view virtual returns (bool pending) {
210: function isOperationReady(bytes32 id) public view virtual returns (bool ready) {
218: function isOperationDone(bytes32 id) public view virtual returns (bool done) {
226: function getTimestamp(bytes32 id) public view virtual returns (uint256 timestamp) {
235: function getMinDelay() public view virtual returns (uint256 duration) {
247: ) public pure virtual returns (bytes32 hash) {[196, 203, 210, 218, 226, 235, 247]
Calling an external function internally, through the use of this wastes the gas overhead of calling an external function (100 gas). Instead, change the function from external to public, and remove the this
There are 3 instances of this issue.
File: src/RBACTimelock.sol
374: return this.onERC721Received.selector;
387: return this.onERC1155Received.selector;
400: return this.onERC1155BatchReceived.selector;Function that are public/external and public state variable names can be optimized to save gas.
Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted. Reference
There are 4 instances of this issue.
File: src/ARMProxy.sol
9: contract ARMProxy is OwnerIsCreator, TypeAndVersionInterface {[9]
File: src/CallProxy.sol
6: contract CallProxy {[6]
File: src/ManyChainMultiSig.sol
44: contract ManyChainMultiSig is Ownable2Step {[44]
File: src/RBACTimelock.sol
50: contract RBACTimelock is AccessControlEnumerable, IERC721Receiver, IERC1155Receiver {[50]
If a string can fit in 32 bytes is it better to use bytes32 instead of string, as it is cheaper.
// @audit avoid this
string constant stringVariable = "LessThan32Bytes";
// @audit as this is cheaper
bytes32 constant stringVariable = "LessThan32Bytes";There is 1 instance of this issue.
File: src/ARMProxy.sol
16: string public constant override typeAndVersion = "ARMProxy 1.0.0";[16]
payable functions cost less gas to execute, since the compiler does not have to add extra checks to ensure that a payment wasn't provided.
A constructor can safely be marked as payable, since only the deployer would be able to pass funds, and the project itself would not pass any funds.
There are 3 instances of this issue.
File: src/ARMProxy.sol
21: constructor(address arm) {[21]
File: src/CallProxy.sol
11: constructor(address target) {[11]
File: src/RBACTimelock.sol
128: constructor([128]
Considering refactoring the revert message to fit in 32 bytes to avoid using more than one memory slot.
There are 6 instances of this issue.
File: src/RBACTimelock.sol
279: require(!isOperation(id), "RBACTimelock: operation already scheduled");
292: require(isOperationPending(id), "RBACTimelock: operation cannot be cancelled");
332: require(success, "RBACTimelock: underlying transaction reverted");
339: require(isOperationReady(id), "RBACTimelock: operation is not ready");
347: require(isOperationReady(id), "RBACTimelock: operation is not ready");
489: require(!_blockedFunctionSelectors.contains(bytes32(selector)), "RBACTimelock: selector is blocked");[279, 292, 332, 339, 347, 489]
Use unchecked to increment the loop variable as it can save gas:
for(uint256 i; i < length;) {
unchecked{
++i;
}
}There are 12 instances of this issue.
File: src/ManyChainMultiSig.sol
217: for (uint256 i = 0; i < signatures.length; i++) {
412: for (uint256 i = 0; i < signerGroups.length; i++) {
420: for (uint256 j = 0; j < NUM_GROUPS; j++) {
445: for (uint256 i = 0; i < oldSigners.length; i++) {
459: for (uint256 i = 0; i < signerAddresses.length; i++) {File: src/RBACTimelock.sol
145: for (uint256 i = 0; i < proposers.length; ++i) {
150: for (uint256 i = 0; i < executors.length; ++i) {
155: for (uint256 i = 0; i < cancellers.length; ++i) {
160: for (uint256 i = 0; i < bypassers.length; ++i) {
269: for (uint256 i = 0; i < calls.length; ++i) {
318: for (uint256 i = 0; i < calls.length; ++i) {
474: for (uint256 i = 0; i < calls.length; ++i) {[145, 150, 155, 160, 269, 318, 474]
Checking for != 0 is cheaper than > 0 for unsigned integers.
There is 1 instance of this issue.
File: src/RBACTimelock.sol
197: return getTimestamp(id) > 0;[197]
A simple zero address check can be written in assembly to save some gas.
There is 1 instance of this issue.
File: src/ARMProxy.sol
28: if (arm == address(0)) revert ZeroAddressNotAllowed();[28]