| Number | Details | Instances |
|---|---|---|
| [MEDIUM-1] | Privileged functions can create points of failure | 9 |
| Number | Details | Instances |
|---|---|---|
| [Low-1] | Gas grief possible on unsafe external calls | 2 |
| [Low-2] | Code does not follow the best practice of check-effects-interaction | 2 |
| [Low-3] | Comparisons against msg.sender and address(this) can easily be bypassed | 2 |
| [Low-4] | Non payable fallback function can break compatibility | 1 |
| [Low-5] | Usage of ecrecover is vulnerable to signature malleability | 1 |
| [Low-6] | Low level calls in solidity versions preceding 0.8.14 can result in an optimiser bug | 3 |
| [Low-7] | Experimental functionality should not be used in production code | 1 |
| [Low-8] | Ownable2Step should be used in place of Ownable | 1 |
| [Low-9] | The call abi.encodeWithSignature is not safe from typographical errors | 1 |
| [Low-10] | Empty fallback functions can cause gas issues | 1 |
| [Low-11] | Uses of EIP712 does not include a version string | 1 |
| [Low-12] | Initializer function can be front run | 1 |
| [Low-13] | Missing zero address check in constructor | 5 |
| [Low-14] | Use of onlyOwner functions can be lost | 1 |
| [Low-15] | Remaining eth may not be refunded to users | 1 |
| [Low-16] | Critical functions should have a timelock | 4 |
| [Low-17] | Missing contract-existence checks before low-level calls | 1 |
| [Low-18] | Use of abi.encodePacked with dynamic types inside keccak256 | 4 |
| [Low-19] | Vulnerable version of openzeppelin contracts used | 1 |
| [Low-20] | Contract contains payable functions but no withdraw/sweep function | 5 |
| [Low-21] | State variables not capped at reasonable values | 3 |
| [Low-22] | The use of deprecated AccessControl functions | 1 |
| [Low-23] | Uses of EIP712 does not include a salt | 1 |
| [Low-24] | Off-by-one timestamp error | 3 |
| [Low-25] | Owner can renounce while system is paused | 1 |
| [Low-26] | Numbers downcast to addresses may result in collisions | 1 |
| [Low-27] | Delegate call can fail | 2 |
| Number | Details | Instances |
|---|---|---|
| [NonCritical-1] | Having chainId as a parameter can introduce cross chain replay attacks | 2 |
| [NonCritical-2] | Use solidity time variables instead of using literals | 2 |
| [NonCritical-3] | Storage Write Removal Bug On Conditional Early Termination | 1 |
| [NonCritical-4] | Some if-statement can be converted to a ternary | 6 |
| [NonCritical-5] | Events may be emitted out of order due to code not follow the best practice of check-effects-interaction | 2 |
| [NonCritical-6] | Consider using time variables when defining time related variables | 3 |
| [NonCritical-7] | Unnecessary struct attribute prefix | 3 |
| [NonCritical-8] | Contracts should have all public/external functions exposed by interfaces | 104 |
| [NonCritical-9] | Using abi.encodePacked can result in hash collision when used in hashing functions | 1 |
| [NonCritical-10] | Consider making private state variables internal to increase flexibility | 2 |
| [NonCritical-11] | Using zero as a parameter | 8 |
| [NonCritical-12] | Consider implementing two-step procedure for updating protocol addresses | 6 |
| [NonCritical-13] | Prefer skip over revert model in iteration | 1 |
| [NonCritical-14] | Floating pragma should be avoided | 1 |
| [NonCritical-15] | Interfaces should be declared in a separate file | 2 |
| [NonCritical-16] | Events regarding state variable changes should emit the previous state variable value | 4 |
| [NonCritical-17] | In functions which accept an address as a parameter, there should be a zero address check to prevent bugs | 39 |
| [NonCritical-18] | Enum values should be used in place of constant array indexes | 1 |
| [NonCritical-19] | Default address values are manually set | 3 |
| [NonCritical-20] | Revert statements within external and public functions can be used to perform DOS attacks | 1 |
| [NonCritical-21] | Reverts should use customer errors instead of strings | 1 |
| [NonCritical-22] | Functions which are either public or external should not have a preceding _ in their name | 15 |
| [NonCritical-23] | Functions which are either private or internal should have a preceding _ in their name | 25 |
| [NonCritical-24] | Contract lines should not be longer than 120 characters for readability | 51 |
| [NonCritical-25] | Avoid updating storage when the value hasn't changed | 4 |
| [NonCritical-26] | Specific imports should be used where possible so only used code is imported | 8 |
| [NonCritical-27] | Old Solidity version | 4 |
| [NonCritical-28] | Not all event definitions are utilizing indexed variables. | 38 |
| [NonCritical-29] | It is convention to make the array size of __gap 50 | 1 |
| [NonCritical-30] | uint/int variables should have the bit size defined explicitly | 70 |
| [NonCritical-31] | Functions within contracts are not ordered according to the solidity style guide | 7 |
| [NonCritical-32] | Double type casts create complexity within the code | 1 |
| [NonCritical-33] | Emits without msg.sender parameter | 14 |
| [NonCritical-34] | Interface imports should be declared first | 6 |
| [NonCritical-35] | Multiple mappings can be replaced with a single struct mapping | 8 |
| [NonCritical-36] | Constants should be on the left side of the comparison | 5 |
| [NonCritical-37] | Interface names should have an I as the first character | 5 |
| [NonCritical-38] | Initialize functions do not emit an event | 3 |
| [NonCritical-39] | Both immutable and constant state variables should be CONSTANT_CASE | 3 |
| [NonCritical-40] | Consider using named mappings | 10 |
| [NonCritical-41] | Use a single file for system wide constants | 6 |
| [NonCritical-42] | Use of non-named numeric constants | 7 |
| [NonCritical-43] | Redundant else statement | 6 |
| [NonCritical-44] | Inconsistent usage of int/uint with int256/uint256 in contract/abstract/library/interface | 7 |
| [NonCritical-45] | Use immutable not constant for keccak state variables | 2 |
| [NonCritical-46] | Consider adding emergency-stop functionality | 19 |
| [NonCritical-47] | Employ Explicit Casting to Bytes or Bytes32 for Enhanced Code Clarity and Meaning | 8 |
| [NonCritical-48] | Non-assembly method available | 1 |
| [NonCritical-49] | Empty bytes check is missing | 19 |
| [NonCritical-50] | Top level declarations should be separated by two blank lines | 13 |
| [NonCritical-51] | Assembly block creates dirty bits | 1 |
| [NonCritical-52] | Whitespace in expressions | 3 |
| [NonCritical-53] | Use of override is unnecessary | 2 |
| [NonCritical-54] | No access control on receive/payable fallback | 1 |
| [NonCritical-55] | Cyclomatic complexity in functions | 7 |
| [NonCritical-56] | Contract uses both require()/revert() as well as custom errors | 2 |
| [NonCritical-57] | function names should be lowerCamelCase | 3 |
| [NonCritical-58] | Consider bounding input array length | 9 |
| [NonCritical-59] | Missing events in sensitive functions | 3 |
| [NonCritical-60] | Consider implementing EIP-5267 to securely describe EIP-712 domains being used | 1 |
| [NonCritical-61] | Ensure block.timestamp is only used in long time intervals | 1 |
| [NonCritical-62] | Contracts with only unimplemented functions can be labeled as abstract | 9 |
| [NonCritical-63] | A event should be emitted if a non immutable state variable is set in a constructor | 8 |
| [NonCritical-64] | Superfluous parameter can only be one value | 1 |
| [NonCritical-65] | Consider only defining one library/interface/contract per sol file | 4 |
| [NonCritical-66] | Using Low-Level Call for Transfers | 1 |
| [NonCritical-67] | Long numbers should include underscores to improve readability and prevent typos | 1 |
| [NonCritical-68] | package.json name variable should only consist of lowercase letters and underscores | 1 |
| [NonCritical-69] | package.json missing/empty description | 1 |
| [NonCritical-70] | Avoid revertible function calls in a constructor | 2 |
| [NonCritical-71] | Avoid declaring variables with the names of defined functions within the project | 4 |
| [NonCritical-72] | Consider using the Upgradeable version of the OpenZeppelin libraries/contracts | 6 |
| [NonCritical-73] | Simplify complex require statements | 2 |
| [NonCritical-74] | Constructors should emit an event | 12 |
| [NonCritical-75] | Contract and Abstract files should have a fixed compiler version | 14 |
| [NonCritical-76] | Function call in event emit | 6 |
| [NonCritical-77] | Consider using AccessControlDefaultAdminRules rather than AccessControl | 1 |
| [NonCritical-78] | Errors should have parameters | 2 |
| [NonCritical-79] | Consider using OpenZeppelins SafeCall library when making calls to arbitrary contracts | 3 |
| [NonCritical-80] | Memory-safe annotation missing | 1 |
| [NonCritical-81] | Constant state variables defined more than once | 12 |
| [NonCritical-82] | Function control structures do not follow the Solidity Style Guide | 2 |
| Number | Details | Instances | Gas |
|---|---|---|---|
| [Gas-1] | State variables used within a function more than once should be cached to save gas | 1 | 1200 |
| [Gas-2] | The result of a function call should be cached rather than re-calling the function | 2 | 800 |
| [Gas-3] | Multiple accesses of the same mapping/array key/index should be cached | 3 | 1134 |
| [Gas-4] | bytes.concat() can be used in place of abi.encodePacked | 1 | 0.0 |
| [Gas-5] | Shortcircuit rules can be be used to optimize some gas usage | 2 | 50400 |
| [Gas-6] | Function calls within for loops | 11 | 0.0 |
| [Gas-7] | For loops in public or external functions should be avoided due to high gas costs and possible DOS | 18 | 0.0 |
| [Gas-8] | Stack variable cost less than structs while used in emiting event | 3 | 81 |
| [Gas-9] | Use assembly to write address/contract storage values | 36 | 0.0 |
| [Gas-10] | Avoid caching global vars used once within the function | 2 | 48 |
| [Gas-11] | There is a 32 byte length threshold for error strings, strings longer than this consume more gas | 77 | 83006 |
| [Gas-12] | Public functions not used internally can be marked as external to save gas | 28 | 0.0 |
| [Gas-13] | Calldata should be used in place of memory function parameters when not mutated | 5 | 325 |
| [Gas-14] | Usage of smaller uint/int types causes overhead | 33 | 59895 |
| [Gas-15] | Use != 0 instead of > 0 | 4 | 48 |
| [Gas-16] | Integer increments by one can be unchecked to save on gas fees | 19 | 43320 |
| [Gas-17] | Use byte32 in place of string | 4 | 0.0 |
| [Gas-18] | Default bool values are manually reset | 2 | 0.0 |
| [Gas-19] | Default int values are manually reset | 1 | 0.0 |
| [Gas-20] | Mappings used within a function more than once should be cached to save gas | 4 | 1600 |
| [Gas-21] | Use assembly to check for the zero address | 17 | 0.0 |
| [Gas-22] | Structs can be packed into fewer storage slots | 2 | 10000 |
| [Gas-23] | Use bitmap to save gas | 8 | 4480 |
| [Gas-24] | Use assembly hashing | 10 | 0.0 |
| [Gas-25] | Use assembly to emit events | 47 | 83942 |
| [Gas-26] | Use assembly in place of abi.decode to extract calldata values more efficiently | 4 | 0.0 |
| [Gas-27] | Counting down in for statements is more gas efficient | 15 | 0.0 |
| [Gas-28] | Using private rather than public for constants and immutables, saves gas | 15 | 0.0 |
| [Gas-29] | Mark Functions That Revert For Normal Users As payable | 9 | 2025 |
| [Gas-30] | Function names can be optimized | 23 | 67712 |
| [Gas-31] | Consider migrating require statements to custom errors | 129 | 232974 |
| [Gas-32] | Refactor event to avoid emitting empty data | 4 | 6000 |
| [Gas-33] | Avoid indexing dynamic types | 2 | 0.0 |
| [Gas-34] | Where a value is casted more than once, consider caching the result to save gas | 3 | 0.0 |
| [Gas-35] | The following mappings can be replaced with a bit mask | 1 | 0.0 |
| [Gas-36] | Unnecessary casting as variable is already of the same type | 2 | 88 |
| [Gas-37] | Simple checks for zero uint can be done using assembly to save gas | 13 | 1014 |
| [Gas-38] | Using nested if to save gas | 2 | 132 |
| [Gas-39] | Consider splitting complex if statements into multiple steps | 4 | 0.0 |
| [Gas-40] | Stack variable cost less than state variables while used in emiting event | 14 | 1764 |
| [Gas-41] | Stack variable cost less than mappings while used in emiting event | 3 | 81 |
| [Gas-42] | Avoid emitting event on every iteration | 1 | 750 |
| [Gas-43] | Low level call can be optimized with assembly | 7 | 12152 |
| [Gas-44] | Constants are cheaper than Enums | 4 | 320000 |
| [Gas-45] | ++X costs slightly less gas than X++ (same with --) | 2 | 20 |
| [Gas-46] | Solidity versions 0.8.19 and above are more gas efficient | 3 | 12000 |
| [Gas-47] | Variable declared within iteration | 1 | 0.0 |
| [Gas-48] | Calling .length in a for loop wastes gas | 8 | 6208 |
| [Gas-49] | Internal functions only used once can be inlined to save gas | 8 | 1920 |
| [Gas-50] | Constructors can be marked as payable to save deployment gas | 12 | 0.0 |
| [Gas-51] | Use assembly scratch space to build calldata for external calls | 3 | 1980 |
| [Gas-52] | Use assembly scratch space to build calldata for event emits | 29 | 185020 |
| [Gas-53] | Assigning to structs can be more efficient | 5 | 3250 |
| [Gas-54] | There are comparisons to boolean literals (true and false), these can be simplified to save gas | 2 | 72 |
| [Gas-55] | Only emit event in setter function if the state variable was changed | 4 | 0.0 |
| [Gas-56] | It is a waste of GAS to emit variable literals | 4 | 128 |
| [Gas-57] | Use OZ Array.unsafeAccess() to avoid repeated array length checks | 30 | 1890000 |
| [Gas-58] | State variable read in a loop | 3 | 260730 |
| [Gas-59] | Write direct outcome, instead of performing mathematical operations for constant state variables | 3 | 0.0 |
| [Gas-60] | It is more efficient to use block.timestamp directly rather than calling a function to return it | 2 | 0.0 |
| [Gas-61] | Use of memory instead of storage for struct/array state variables | 3 | 18900 |
| [Gas-62] | Public functions not called internally | 28 | 0.0 |
| [Gas-63] | do-while is cheaper than for-loops when the initial check can be skipped | 16 | 44800 |
| [Gas-64] | Empty blocks should be removed or emit something | 3 | 0.0 |
9
Ensure such accounts are protected and consider implementing multi sig to prevent a single point of failure
Findings are labeled with ' <= FOUND'
Click to show findings
93: function setAccessControlManager(address accessControlManager_) external onlyOwner // <= FOUND48: function setMaxDailyReceiveLimit(uint256 limit_) external onlyOwner // <= FOUND57: function pause() external onlyOwner // <= FOUND65: function unpause() external onlyOwner // <= FOUND93: function setAccessControlManager(address accessControlManager_) external onlyOwner // <= FOUND69: function upsertSignature(string[] calldata signatures_, bool[] calldata active_) external onlyOwner // <= FOUND146: function setSrcChainId(uint16 srcChainId_) external onlyOwner // <= FOUND157: function addTimelocks(ITimelock[] memory timelocks_) external onlyOwner // <= FOUND214: function fallbackWithdraw(
215: address to_,
216: uint256 pId_,
217: uint16 remoteChainId_,
218: bytes calldata payload_,
219: bytes calldata adapterParams_,
220: uint256 originalValue_
221: ) external onlyOwner nonReentrant // <= FOUNDIn Solidity, the use of low-level call methods can expose contracts to gas griefing attacks. The potential problem arises when the callee contract returns a large amount of data. This data is allocated in the memory of the calling contract, which pays for the gas costs. If the callee contract intentionally returns an enormous amount of data, the gas costs can skyrocket, causing the transaction to fail due to an Out of Gas error. Therefore, it's advisable to limit the use of call when interacting with untrusted contracts, or ensure that the callee's returned data size is capped or known in advance to prevent unexpected high gas costs.
Num of instances: 2
Click to show findings
['66']
66: function delegateTo(address callee, bytes memory data) internal { // <= FOUND
67: (bool success, bytes memory returnData) = callee.delegatecall(data); // <= FOUND
68: assembly {
69: if eq(success, 0) {
70: revert(add(returnData, 0x20), returndatasize)
71: }
72: }
73: }
['66']
66: function delegateTo(address callee, bytes memory data) internal { // <= FOUND
67: (bool success, bytes memory returnData) = callee.delegatecall(data); // <= FOUND
68: assembly {
69: if eq(success, 0) {
70: revert(add(returnData, 0x20), returndatasize)
71: }
72: }
73: }
The "check-effects-interaction" pattern is a best practice in smart contract development, emphasizing the order of operations in functions to prevent reentrancy attacks. Violations arise when a function interacts with external contracts before settling internal state changes or checks. This misordering can expose the contract to potential threats. To adhere to this pattern, first ensure all conditions or checks are satisfied, then update any internal states, and only after these steps, interact with external contracts or addresses. Rearranging operations to this recommended sequence bolsters contract security and aligns with established best practices in the Ethereum community.
Num of instances: 2
Click to show findings
['498']
498: function _initiate(address governorAlpha) external { // <= FOUND
499: require(msg.sender == admin, "GovernorBravo::_initiate: admin only");
500: require(initialProposalId == 0, "GovernorBravo::_initiate: can only initiate once");
501: proposalCount = GovernorAlphaInterface(governorAlpha).proposalCount(); // <= FOUND
502: initialProposalId = proposalCount;
503: for (uint256 i; i < uint8(ProposalType.CRITICAL) + 1; ++i) {
504: proposalTimelocks[i].acceptAdmin();
505: }
506: }
['445']
445: function _initiate(address governorAlpha) external { // <= FOUND
446: require(msg.sender == admin, "GovernorBravo::_initiate: admin only");
447: require(initialProposalId == 0, "GovernorBravo::_initiate: can only initiate once");
448: proposalCount = GovernorAlphaInterface(governorAlpha).proposalCount(); // <= FOUND
449: initialProposalId = proposalCount;
450: timelock.acceptAdmin();
451: }
Comparisons using msg.sender and address(this) can be exploited when contracts call their own functions. If an attacker influences the calldata during such a self-call, they can bypass checks expecting msg.sender to be an external address. Instead, msg.sender becomes the contract's own address. This can circumvent security checks designed for external calls or disrupt logic that uses msg.sender for accounting or differentiation. To counter this, contracts should not solely rely on msg.sender checks and should be wary of potential recursive calls. It's crucial to implement additional validation mechanisms and conduct thorough tests to prevent unintended call behaviors.
Num of instances: 2
Click to show findings
['91']
91: require(msg.sender == address(this), "Timelock::setDelay: Call must come from Timelock."); // <= FOUND
['141']
141: require(msg.sender == address(this), "Timelock::setPendingAdmin: Call must come from Timelock."); // <= FOUND
Reason: In Solidity, a defined fallback function that is not declared as payable can cause compatibility issues when value is transferred with incorrect data. If the contract is meant to receive Ether but the fallback function isn't payable, the contract will reject the transaction, resulting in a failed operation. This could lead to unintentional loss of funds or incompatible interactions with other contracts.
Resolution: To ensure broad compatibility and safe receipt of Ether, the fallback function should be declared as payable. This allows the contract to accept Ether transfers, regardless of the data sent along with the transaction. However, it's important to note that this should be complemented by proper checks and controls in the contract to avoid potential attacks or misuse. Always exercise caution when dealing with payable functions, and consider using the latest Solidity version to take advantage of the receive() function for explicit Ether transfers.
Num of instances: 1
Click to show findings
['54']
54: fallback(bytes calldata data_) external returns (bytes memory) {
55: string memory fun = functionRegistry[msg.sig];
56: require(bytes(fun).length != 0, "Function not found");
57: _checkAccessAllowed(fun);
58: (bool ok, bytes memory res) = address(OMNICHAIN_GOVERNANCE_EXECUTOR).call(data_);
59: require(ok, "call failed");
60: return res;
61: }
The ecrecover function in Ethereum smart contracts is vulnerable to signature malleability due to the nature of Ethereum signatures and the ECDSA (Elliptic Curve Digital Signature Algorithm) used. Ethereum signatures include an additional field, v (the recovery identifier), which can have multiple valid values for the same signer and message. This field, which ranges from 27 to 30, helps to determine the correct public key associated with a signature. The vulnerability arises because there are multiple valid s values for any given r in a signature, meaning (r, s) and (r, n - s) can both be valid for the same message and signing key. The ecrecover function, which is used to verify signatures and recover addresses, must account for these different valid s values, leading to potential signature malleability issues.
Num of instances: 1
In Solidity versions 0.8.13 and 0.8.14, a known optimizer bug presents potential risks when a variable is used in a separate assembly block from the one in which it was stored. Specifically, the 'mstore' operation could be optimized out due to this bug, leading to the use of uninitialized memory. Although the current code does not exhibit this risky pattern of execution, it does utilize 'mstore' within assembly blocks, which introduces a vulnerability risk for future code modifications. As a preventative measure, it is advisable to avoid the usage of the afflicted Solidity versions, 0.8.13 and 0.8.14. Instead, consider utilizing a version that is not impacted by this optimizer bug to prevent potential memory initialization issues in your smart contract.
Num of instances: 3
Click to show findings
['68']
68: assembly { // <= FOUND
69: if eq(success, 0) {
70: revert(add(returnData, 0x20), returndatasize)
71: }
72: }
['84']
84: assembly { // <= FOUND
85: let free_mem_ptr := mload(0x40)
86: returndatacopy(free_mem_ptr, 0, returndatasize)
87:
88: switch success
89: case 0 {
90: revert(free_mem_ptr, returndatasize)
91: }
92: default {
93: return(free_mem_ptr, returndatasize)
94: }
95: }
['384']
384: assembly { // <= FOUND
385: chainId := chainid()
386: }
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
Ownable2Step further prevents risks posed by centralised privileges as there is a smaller likelihood of the owner being wrongfully changed
Num of instances: 1
Click to show findings
['17']
17: contract BaseOmnichainControllerSrc is Ownable, Pausable // <= FOUND
When you use abi.encodeWithSignature in Solidity, you're creating an ABI-encoded string representing a function call. This method relies on you, the programmer, correctly typing the function signature (i.e., the function name and types of arguments) as a string.
For instance, if you want to call a function foo(uint256, address), you'd write abi.encodeWithSignature("foo(uint256,address)", arg1, arg2). If there's a typographical error like abi.encodeWithSignature("foo(uinnt256,address)", arg1, arg2), the Solidity compiler will not raise an error because the string is syntactically correct. However, this will fail at runtime because there's no function with that incorrect signature.
This makes abi.encodeWithSignature less safe than directly calling the function or using interfaces to call functions on other contracts. An effective resolution would be using a Solidity interface. An interface provides compile-time type checking, ensuring that you're calling existing functions with the correct parameters.
Num of instances: 1
Click to show findings
['27']
25: delegateTo(
26: implementation_,
27: abi.encodeWithSignature( // <= FOUND
28: "initialize(address,address,uint256,uint256,uint256,address)",
29: timelock_,
30: xvsVault_,
31: votingPeriod_,
32: votingDelay_,
33: proposalThreshold_,
34: guardian_
35: )
36: );
An empty fallback() function in a Solidity contract can cause issues, as it might inadvertently accept Ether transfers without any mechanism to manage or allocate the received funds. Consequently, Ether could become permanently locked within the contract, causing potential financial losses and diminishing user trust.
To rectify this situation, developers should implement appropriate logic within the fallback() function to handle incoming Ether transfers. This may include updating internal balances, emitting events for transparency, or implementing access control mechanisms to restrict Ether transfers to authorized parties
Num of instances: 1
It is standard for uses of EIP712 to include a version string, not doing so can cause future incompatibilities
Num of instances: 1
Click to show findings
['107']
105:
106: bytes32 public constant DOMAIN_TYPEHASH =
107: keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)"); // <= FOUND
In Solidity contract deployment, not making the initialize() function call atomic with the contract creation can leave a vulnerability window. A malicious actor could exploit this time gap and call initialize() before the intended initialization. This action could disrupt the contract's setup, potentially necessitating a full contract re-deployment to ensure proper initialization. To mitigate such risks, it's advised to use a factory contract. This factory contract can be programmed to deploy and initialize a new contract in a single atomic transaction, closing the window of vulnerability and ensuring correct and secure contract initialization.
Num of instances: 1
Click to show findings
['43']
43: function initialize(address accessControlManager_) external initializer // <= FOUND
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: 5
Click to show findings
['13']
12: constructor(
13: address timelock_, // <= FOUND
14: address xvsVault_, // <= FOUND
15: address admin_, // <= FOUND
16: address implementation_, // <= FOUND
17: uint votingPeriod_,
18: uint votingDelay_,
19: uint proposalThreshold_,
20: address guardian_ // <= FOUND
21: ) public {
22:
23: admin = msg.sender;
24:
25: delegateTo(
26: implementation_,
27: abi.encodeWithSignature(
28: "initialize(address,address,uint256,uint256,uint256,address)",
29: timelock_,
30: xvsVault_,
31: votingPeriod_,
32: votingDelay_,
33: proposalThreshold_,
34: guardian_
35: )
36: );
37:
38: _setImplementation(implementation_);
39:
40: admin = admin_;
41: }
['136']
136: constructor(address timelock_, address xvs_, address guardian_) public { // <= FOUND
137: timelock = TimelockInterface(timelock_);
138: xvs = XVSInterface(xvs_);
139: guardian = guardian_;
140: }
['136']
136: constructor(address timelock_, address xvs_, address guardian_, uint256 lastProposalId_) public { // <= FOUND
137: timelock = TimelockInterface(timelock_);
138: xvs = XVSInterface(xvs_);
139: guardian = guardian_;
140: proposalCount = lastProposalId_;
141: }
['13']
12: constructor(
13: address timelock_, // <= FOUND
14: address xvsVault_, // <= FOUND
15: address admin_, // <= FOUND
16: address implementation_, // <= FOUND
17: uint votingPeriod_,
18: uint votingDelay_,
19: uint proposalThreshold_,
20: address guardian_ // <= FOUND
21: ) public {
22:
23: admin = msg.sender;
24:
25: delegateTo(
26: implementation_,
27: abi.encodeWithSignature(
28: "initialize(address,address,uint256,uint256,uint256,address)",
29: timelock_,
30: xvsVault_,
31: votingPeriod_,
32: votingDelay_,
33: proposalThreshold_,
34: guardian_
35: )
36: );
37:
38: _setImplementation(implementation_);
39:
40: admin = admin_;
41: }
['72']
72: constructor(address admin_, uint delay_) public { // <= FOUND
73: require(delay_ >= MINIMUM_DELAY, "Timelock::constructor: Delay must exceed minimum delay.");
74: require(delay_ <= MAXIMUM_DELAY, "Timelock::setDelay: Delay must not exceed maximum delay.");
75:
76: admin = admin_;
77: delay = delay_;
78: }
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: 1
Click to show findings
['17']
17: contract BaseOmnichainControllerSrc is Ownable, Pausable // <= FOUND
When a contract function accepts Ethereum and executes a .call() or similar function that also forwards Ethereum value, it's important to check for and refund any remaining balance. This is because some of the supplied value may not be used during the call execution due to gas constraints, a revert in the called contract, or simply because not all the value was needed.
If you do not account for this remaining balance, it can become "locked" in the contract. It's crucial to either return the remaining balance to the sender or handle it in a way that ensures it is not permanently stuck. Neglecting to do so can lead to loss of funds and degradation of the contract's reliability. Furthermore, it's good practice to ensure fairness and trust with your users by returning unused funds.
Num of instances: 1
Click to show findings
['217']
217: function executeTransaction(
218: address target,
219: uint256 value,
220: string calldata signature,
221: bytes calldata data,
222: uint256 eta
223: ) public payable returns (bytes memory) { // <= FOUND
224: require(msg.sender == admin, "Timelock::executeTransaction: Call must come from admin.");
225:
226: bytes32 txHash = keccak256(abi.encode(target, value, signature, data, eta));
227: require(queuedTransactions[txHash], "Timelock::executeTransaction: Transaction hasn't been queued.");
228: require(getBlockTimestamp() >= eta, "Timelock::executeTransaction: Transaction hasn't surpassed time lock.");
229: require(getBlockTimestamp() <= eta + GRACE_PERIOD(), "Timelock::executeTransaction: Transaction is stale.");
230:
231: delete (queuedTransactions[txHash]);
232:
233: bytes memory callData;
234:
235: if (bytes(signature).length == 0) {
236: callData = data;
237: } else {
238: callData = abi.encodePacked(bytes4(keccak256(bytes(signature))), data);
239: }
240:
241:
242: (bool success, bytes memory returnData) = target.call{ value: value }(callData); // <= FOUND
243: require(success, "Timelock::executeTransaction: Transaction execution reverted.");
244:
245: emit ExecuteTransaction(txHash, target, value, signature, data, eta);
246:
247: return returnData;
248: }
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: 4
Click to show findings
['93']
93: function setAccessControlManager(address accessControlManager_) external onlyOwner // <= FOUND
['48']
48: function setMaxDailyReceiveLimit(uint256 limit_) external onlyOwner // <= FOUND
['93']
93: function setAccessControlManager(address accessControlManager_) external onlyOwner // <= FOUND
['146']
146: function setSrcChainId(uint16 srcChainId_) external onlyOwner // <= FOUND
Low-level calls in Solidity, when made to addresses without contract code, don't fail but return a successful status. This behavior can be misleading, leading to unintended consequences in dApps. Ignoring this can potentially mean acting on false positive results. To address this, apart from the conventional zero-address check, developers should verify the existence of contract code at the target address by ensuring that the code length at the specified address (<address>.code.length) is greater than zero. By doing so, it provides a more robust validation before executing low-level calls, safeguarding against unintentional interactions with empty addresses.
Num of instances: 1
Click to show findings
['217']
217: function executeTransaction(
218: address target,
219: uint256 value,
220: string calldata signature,
221: bytes calldata data,
222: uint256 eta
223: ) public payable returns (bytes memory) {
224: require(msg.sender == admin, "Timelock::executeTransaction: Call must come from admin.");
225:
226: bytes32 txHash = keccak256(abi.encode(target, value, signature, data, eta));
227: require(queuedTransactions[txHash], "Timelock::executeTransaction: Transaction hasn't been queued.");
228: require(getBlockTimestamp() >= eta, "Timelock::executeTransaction: Transaction hasn't surpassed time lock.");
229: require(getBlockTimestamp() <= eta + GRACE_PERIOD(), "Timelock::executeTransaction: Transaction is stale.");
230:
231: delete (queuedTransactions[txHash]);
232:
233: bytes memory callData;
234:
235: if (bytes(signature).length == 0) {
236: callData = data;
237: } else {
238: callData = abi.encodePacked(bytes4(keccak256(bytes(signature))), data);
239: }
240:
241:
242: (bool success, bytes memory returnData) = target.call{ value: value }(callData); // <= FOUND
243: require(success, "Timelock::executeTransaction: Transaction execution reverted.");
244:
245: emit ExecuteTransaction(txHash, target, value, signature, data, eta);
246:
247: return returnData;
248: }
Using abi.encodePacked with dynamic types for hashing functions like keccak256 can be risky due to the potential for hash collisions. This function concatenates arguments tightly, without padding, which might lead to different inputs producing the same hash. This is especially problematic with dynamic types, where the boundaries between inputs can blur. To mitigate this, use abi.encode instead. abi.encode pads its arguments to 32 bytes, creating clear distinctions between different inputs and significantly reducing the chance of hash collisions. This approach ensures more reliable and collision-resistant hashing, crucial for maintaining data integrity and security in smart contracts.
Num of instances: 4
Click to show findings
['78']
78: bytes32 role = keccak256(abi.encodePacked(contractAddress, functionSig)); // <= FOUND
['110']
110: bytes32 role = keccak256(abi.encodePacked(msg.sender, functionSig)); // <= FOUND
['115']
115: role = keccak256(abi.encodePacked(address(0), functionSig)); // <= FOUND
['325']
325: bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash)); // <= 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
Click to show findings
['[1](project_package.txt: 1-62)']
1: {
2: "name": "@venusprotocol/governance-contracts",
3: "description": "",
4: "version": "1.4.0",
5: "author": "",
6: "files": [
7: "artifacts",
8: "dist",
9: "deploy",
10: "deployments",
11: "contracts"
12: ],
13: "keywords": [
14: "blockchain",
15: "ethers",
16: "ethereum",
17: "hardhat",
18: "smart-contracts",
19: "solidity",
20: "template",
21: "typescript",
22: "typechain"
23: ],
24: "packageManager": "yarn@3.2.0",
25: "publishConfig": {
26: "access": "public"
27: },
28: "scripts": {
29: "compile": "hardhat compile",
30: "test": "hardhat test",
31: "build": "rm -rf dist && tsc --declaration && hardhat compile",
32: "publish:dist": "yarn build && cd dist && yarn publish --access public",
33: "lint": "yarn lint:ts && yarn lint:sol && yarn prettier:check",
34: "lint:ts": "eslint --ignore-path ./.eslintignore --ext .js,.ts .",
35: "lint:sol": "solhint \"contracts/**/*.sol\"",
36: "lint:sol:fix": "prettier --write \"contracts/**/*.sol\"",
37: "prettier": "prettier --write \"**/*.{js,json,md,ts,yaml,yml,sol}\"",
38: "prettier:check": "prettier --check \"**/*.{js,json,md,ts,yaml,yml,sol}\"",
39: "docgen": "hardhat docgen",
40: "prepare": "husky install"
41: },
42: "dependencies": {
43: "@venusprotocol/solidity-utilities": "1.3.0",
44: "hardhat-deploy-ethers": "^0.3.0-beta.13",
45: "module-alias": "^2.2.2"
46: },
47: "devDependencies": {
48: "@commitlint/cli": "^17.4.4",
49: "@commitlint/config-conventional": "^17.4.4",
50: "@defi-wonderland/smock": "^2.3.4",
51: "@ethersproject/abi": "^5.7.0",
52: "@ethersproject/abstract-signer": "^5.7.0",
53: "@ethersproject/bignumber": "^5.7.0",
54: "@ethersproject/bytes": "^5.7.0",
55: "@ethersproject/providers": "^5.7.2",
56: "@layerzerolabs/solidity-examples": "^1.0.0",
57: "@nomicfoundation/hardhat-chai-matchers": "^1.0.4",
58: "@nomicfoundation/hardhat-network-helpers": "^1.0.6",
59: "@nomicfoundation/hardhat-toolbox": "^2.0.0",
60: "@nomiclabs/hardhat-ethers": "^2.2.1",
61: "@nomiclabs/hardhat-etherscan": "^3.1.2",
62: "@openzeppelin/contracts": "^4.8.2", // <= FOUND
63: "@openzeppelin/contracts-upgradeable": "^4.8.2",
64: "@openzeppelin/hardhat-upgrades": "^1.22.1",
65: "@semantic-release/changelog": "^6.0.2",
66: "@semantic-release/git": "^10.0.1",
67: "@trivago/prettier-plugin-sort-imports": "^4.0.0",
68: "@typechain/ethers-v5": "^10.1.1",
69: "@typechain/hardhat": "^6.1.4",
70: "@types/chai": "^4.3.4",
71: "@types/fs-extra": "^9.0.13",
72: "@types/mocha": "^10.0.0",
73: "@types/node": "^18.16.3",
74: "@typescript-eslint/eslint-plugin": "^5.44.0",
75: "@typescript-eslint/parser": "^5.44.0",
76: "@venusprotocol/venus-protocol": "7.4.0",
77: "bignumber.js": "^9.1.1",
78: "chai": "^4.3.7",
79: "commitizen": "^4.2.5",
80: "cross-env": "^7.0.3",
81: "cz-conventional-changelog": "^3.3.0",
82: "dotenv": "^16.0.3",
83: "eslint": "^8.28.0",
84: "eslint-config-prettier": "^8.5.0",
85: "ethers": "^5.7.2",
86: "fs-extra": "^10.1.0",
87: "hardhat": "^2.16.1",
88: "hardhat-dependency-compiler": "^1.1.3",
89: "hardhat-deploy": "^0.11.14",
90: "hardhat-docgen": "^1.3.0",
91: "hardhat-gas-reporter": "^1.0.9",
92: "husky": "^8.0.3",
93: "lint-staged": "^13.0.4",
94: "lodash": "^4.17.21",
95: "mocha": "^10.1.0",
96: "pinst": "^3.0.0",
97: "prettier": "^2.8.4",
98: "prettier-plugin-solidity": "^1.1.2",
99: "semantic-release": "^19.0.3",
100: "shx": "^0.3.4",
101: "solhint": "^3.4.0",
102: "solhint-plugin-prettier": "^0.0.5",
103: "solidity-coverage": "^0.8.4",
104: "solidity-docgen": "^0.6.0-beta.34",
105: "ts-generator": "^0.1.1",
106: "ts-node": "^10.9.1",
107: "tsconfig-paths": "^4.1.2",
108: "typechain": "^8.1.1",
109: "typescript": "^4.9.3"
110: },
111: "_moduleAliases": {
In smart contract development, particularly for Ethereum, having payable functions without a corresponding withdraw or sweep function can lead to potential issues. Payable functions allow the contract to receive Ether, but without a mechanism to withdraw these funds, the Ether can become locked within the contract indefinitely. This situation might be intentional in some cases (like a burn function), but generally, it’s a design oversight. A withdraw or sweep function is necessary to transfer Ether out of the contract to a specific address, typically the owner's or a designated recipient. Without this, the contract lacks flexibility in managing its funds, potentially leading to lost or inaccessible Ether.
Num of instances: 5
Click to show findings
['4']
4: contract GovernorAlpha
['4']
4: contract GovernorAlpha2
['19']
19: contract OmnichainProposalSender is ReentrancyGuard, BaseOmnichainControllerSrc
['10']
10: contract Timelock
['12']
12: contract TimelockV8
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: 3
Click to show findings
['460']
458: function _setProposalMaxOperations(uint proposalMaxOperations_) external {
459: require(msg.sender == admin, "GovernorBravo::_setProposalMaxOperations: admin only");
460: uint oldProposalMaxOperations = proposalMaxOperations; // <= FOUND
461: proposalMaxOperations = proposalMaxOperations_; // <= FOUND
462:
463: emit ProposalMaxOperationsUpdated(oldProposalMaxOperations, proposalMaxOperations_);
464: }
['50']
48: function setMaxDailyReceiveLimit(uint256 limit_) external onlyOwner {
49: emit SetMaxDailyReceiveLimit(maxDailyReceiveLimit, limit_);
50: maxDailyReceiveLimit = limit_; // <= FOUND
51: }
['65']
63: function setMaxDailyLimit(uint16 chainId_, uint256 limit_) external {
64: _ensureAllowed("setMaxDailyLimit(uint16,uint256)");
65: emit SetMaxDailyLimit(chainId_, chainIdToMaxDailyLimit[chainId_], limit_); // <= FOUND
66: chainIdToMaxDailyLimit[chainId_] = limit_; // <= FOUND
67: }
The '_setupRole' function is deprecated in current OZ AccessControl implementations so it is advisable to not use it to maintain compatability.
Num of instances: 1
It is standard for uses of EIP712 to include a salt, not doing so can cause future incompatibilities and in this instance cause hash collisions do to no salting
Num of instances: 1
Click to show findings
['105']
105:
106: bytes32 public constant DOMAIN_TYPEHASH =
107: keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)"); // <= FOUND
In Solidity, using >= or <= to compare against block.timestamp (alias now) may introduce off-by-one errors due to the fact that block.timestamp is only updated once per block and its value remains constant throughout the block's execution. If an operation happens at the exact second when block.timestamp changes, it could result in unexpected behavior. To avoid this, it's safer to use strict inequality operators (> or <). For instance, if a condition should only be met after a certain time, use block.timestamp > time rather than block.timestamp >= time. This way, potential off-by-one errors due to the exact timing of block mining are mitigated, leading to safer, more predictable contract behavior.
Num of instances: 3
Click to show findings
['294']
294: function state(uint proposalId) public view returns (ProposalState) { // <= FOUND
295: require(proposalCount >= proposalId && proposalId > 0, "GovernorAlpha::state: invalid proposal id");
296: Proposal storage proposal = proposals[proposalId];
297: if (proposal.canceled) {
298: return ProposalState.Canceled;
299: } else if (block.number <= proposal.startBlock) {
300: return ProposalState.Pending;
301: } else if (block.number <= proposal.endBlock) {
302: return ProposalState.Active;
303: } else if (proposal.forVotes <= proposal.againstVotes || proposal.forVotes < quorumVotes()) {
304: return ProposalState.Defeated;
305: } else if (proposal.eta == 0) {
306: return ProposalState.Succeeded;
307: } else if (proposal.executed) {
308: return ProposalState.Executed; // <= FOUND
309: } else if (block.timestamp >= add256(proposal.eta, timelock.GRACE_PERIOD())) { // <= FOUND
310: return ProposalState.Expired;
311: } else {
312: return ProposalState.Queued;
313: }
314: }
['294']
294: function state(uint proposalId) public view returns (ProposalState) { // <= FOUND
295: require(proposalCount >= proposalId && proposalId > 0, "GovernorAlpha::state: invalid proposal id");
296: Proposal storage proposal = proposals[proposalId];
297: if (proposal.canceled) {
298: return ProposalState.Canceled;
299: } else if (block.number <= proposal.startBlock) {
300: return ProposalState.Pending;
301: } else if (block.number <= proposal.endBlock) {
302: return ProposalState.Active;
303: } else if (proposal.forVotes <= proposal.againstVotes || proposal.forVotes < quorumVotes()) {
304: return ProposalState.Defeated;
305: } else if (proposal.eta == 0) {
306: return ProposalState.Succeeded;
307: } else if (proposal.executed) {
308: return ProposalState.Executed; // <= FOUND
309: } else if (block.timestamp >= add256(proposal.eta, timelock.GRACE_PERIOD())) { // <= FOUND
310: return ProposalState.Expired;
311: } else {
312: return ProposalState.Queued;
313: }
314: }
['289']
289: function state(uint proposalId) public view returns (ProposalState) { // <= FOUND
290: require(
291: proposalCount >= proposalId && proposalId > initialProposalId,
292: "GovernorBravo::state: invalid proposal id"
293: );
294: Proposal storage proposal = proposals[proposalId];
295: if (proposal.canceled) {
296: return ProposalState.Canceled;
297: } else if (block.number <= proposal.startBlock) {
298: return ProposalState.Pending;
299: } else if (block.number <= proposal.endBlock) {
300: return ProposalState.Active;
301: } else if (proposal.forVotes <= proposal.againstVotes || proposal.forVotes < quorumVotes) {
302: return ProposalState.Defeated;
303: } else if (proposal.eta == 0) {
304: return ProposalState.Succeeded;
305: } else if (proposal.executed) {
306: return ProposalState.Executed; // <= FOUND
307: } else if (block.timestamp >= add256(proposal.eta, timelock.GRACE_PERIOD())) { // <= FOUND
308: return ProposalState.Expired;
309: } else {
310: return ProposalState.Queued;
311: }
312: }
If an owner renounces their role while a system is paused, it could lead to permanent inaccessibility of assets stored in the contract. Such a scenario jeopardizes the trust and functionality of the protocol, as no one would be able to unpause the system or access funds. To mitigate this risk, it's essential to implement a mechanism that either prevents the owner from renouncing while the system is paused or requires multiple signatories to perform such critical actions. By introducing such safeguards, it ensures that user assets are not rendered inaccessible due to a single user's action or oversight.
Num of instances: 1
Click to show findings
['17']
17: contract BaseOmnichainControllerSrc is Ownable, Pausable // <= FOUND
Downcasting numbers to addresses in blockchain contracts, particularly in Ethereum's Solidity, involves risks such as possible address collisions. A collision occurs when different inputs, when cast or hashed, generate the same output address, potentially compromising contract integrity and asset security. If an uint256, for instance, is downcast to an address (effectively an uint160) without ensuring it’s a legitimate, collision-free conversion, different uint256 inputs might yield the same address, creating vulnerabilities attackers might exploit. Implementing thorough checks and opting for secure practices, like avoiding downcasting in critical logic or utilizing mappings with original uint256 as keys, mitigates risks
Num of instances: 1
Click to show findings
['252']
252: ensureNonzeroAddress(address(uint160(bytes20(newRemoteAddress_)))); // <= FOUND
When a non payable function contains a payable call, it is important to point up that the delegate call will be limited to only calling other non payable functions. This can potentially cause future issues if this is not accounted for.
Num of instances: 2
Click to show findings
['67']
66: function delegateTo(address callee, bytes memory data) internal {
67: (bool success, bytes memory returnData) = callee.delegatecall(data); // <= FOUND
68: assembly {
69: if eq(success, 0) {
70: revert(add(returnData, 0x20), returndatasize)
71: }
72: }
73: }
['67']
66: function delegateTo(address callee, bytes memory data) internal {
67: (bool success, bytes memory returnData) = callee.delegatecall(data); // <= FOUND
68: assembly {
69: if eq(success, 0) {
70: revert(add(returnData, 0x20), returndatasize)
71: }
72: }
73: }
Accepting chainId as a parameter in a Solidity contract could open up possibilities for cross-chain replay attacks. An attacker could execute a transaction on one chain, then 'replay' it on another chain where the contract has been deployed, leading to unintended consequences. It's crucial to get the chainId within the contract using block.chainId to ensure that the transactions are valid and specific to the chain the contract resides on. The block.chainId value is provided by the EVM and can be trusted, as it's difficult to manipulate and is unique to each Ethereum-based network. This enhances the security of cross-chain operations.
Num of instances: 2
Click to show findings
['63']
63: function setMaxDailyLimit(uint16 chainId_, uint256 limit_) external // <= FOUND
['266']
266: function setConfig(uint16 version_, uint16 chainId_, uint256 configType_, bytes calldata config_) external // <= FOUND
Num of instances: 2
Click to show findings
['30']
29: function votingPeriod() public pure returns (uint) {
30: return (60 * 60 * 24 * 3) / 3; // <= FOUND
31: }
['30']
29: function votingPeriod() public pure returns (uint) {
30: return (60 * 60 * 24 * 3) / 3; // <= FOUND
31: }
In September 2022, a bug in Solidity’s Yul optimizer was identified through differential fuzzing. This bug, introduced in version 0.8.13 and fixed by version 0.8.17, can be activated easier with optimized via-IR code generation but can also potentially occur in optimized legacy code generation. This bug is of medium/high severity, which requires contracts that use large inline assembly blocks containing user-defined assembly functions involving return(...) or stop() instructions to be reviewed.
The bug is associated with the Unused Store Eliminator in Yul optimizer, which removes redundant storage writes, and is triggered when function calls are performed. If the function call conditionally continues after the call to a function and terminates using return(...) or stop(), the optimizer may incorrectly remove storage writes before calls to the function.
To evaluate if a contract is affected, you need to check contracts that include inline assembly block with return(...) or stop() statements. If early termination happens conditionally within a function, the optimizer might remove storage writes before function calls.
To avoid this, contracts should not allow for conditional early termination within a function. It is also recommended to use a version of Solidity where this bug has been fixed (version 0.8.17 or later).
Num of instances: 1
Click to show findings
['93']
84: assembly {
85: let free_mem_ptr := mload(0x40)
86: returndatacopy(free_mem_ptr, 0, returndatasize)
87:
88: switch success
89: case 0 {
90: revert(free_mem_ptr, returndatasize)
91: }
92: default {
93: return(free_mem_ptr, returndatasize) // <= FOUND
94: }
95: }
Improving code readability and compactness is an integral part of optimal programming practices. The use of ternary operators in place of if-else conditions is one such measure. Ternary operators allow us to write conditional statements in a more concise manner, thereby enhancing readability and simplicity. They follow the syntax condition ? exprIfTrue : exprIfFalse, which interprets as "if the condition is true, evaluate to exprIfTrue, else evaluate to exprIfFalse". By adopting this approach, we make our code more streamlined and intuitive, which could potentially aid in better understanding and maintenance of the codebase.
Num of instances: 6
Click to show findings
['235']
235: if (bytes(signature).length == 0) {
236: callData = data; // <= FOUND
237: }
['51']
51: if (!isAllowedToCall) { // <= FOUND
52: revert("Unauthorized"); // <= FOUND
53: }
['77']
77: if (!isAllowedToCall) { // <= FOUND
78: revert Unauthorized(msg.sender, address(this), signature); // <= FOUND
79: }
['112']
112: if (hasRole(role, account)) { // <= FOUND
113: return true;
114: }
['297']
297: if (proposal.canceled) { // <= FOUND
298: return ProposalState.Canceled;
299: }
['88']
88: if (a == 0) { // <= FOUND
89: return 0;
90: }
[NonCritical-5] Events may be emitted out of order due to code not follow the best practice of check-effects-interaction
The "check-effects-interaction" pattern also impacts event ordering. When a contract doesn't adhere to this pattern, events might be emitted in a sequence that doesn't reflect the actual logical flow of operations. This can cause confusion during event tracking, potentially leading to erroneous off-chain interpretations. To rectify this, always ensure that checks are performed first, state modifications come next, and interactions with external contracts or addresses are done last. This will ensure events are emitted in a logical, consistent manner, providing a clear and accurate chronological record of on-chain actions for off-chain systems and observers.
Num of instances: 2
Click to show findings
['177']
177: function executeTransaction(
178: address target,
179: uint value,
180: string memory signature,
181: bytes memory data,
182: uint eta
183: ) public payable returns (bytes memory) {
184: require(msg.sender == admin, "Timelock::executeTransaction: Call must come from admin.");
185:
186: bytes32 txHash = keccak256(abi.encode(target, value, signature, data, eta));
187: require(queuedTransactions[txHash], "Timelock::executeTransaction: Transaction hasn't been queued.");
188: require(getBlockTimestamp() >= eta, "Timelock::executeTransaction: Transaction hasn't surpassed time lock.");
189: require(getBlockTimestamp() <= eta.add(GRACE_PERIOD), "Timelock::executeTransaction: Transaction is stale.");
190:
191: queuedTransactions[txHash] = false;
192:
193: bytes memory callData;
194:
195: if (bytes(signature).length == 0) {
196: callData = data;
197: } else {
198: callData = abi.encodePacked(bytes4(keccak256(bytes(signature))), data);
199: }
200:
201:
202: (bool success, bytes memory returnData) = target.call.value(value)(callData); // <= FOUND
203: require(success, "Timelock::executeTransaction: Transaction execution reverted.");
204:
205: emit ExecuteTransaction(txHash, target, value, signature, data, eta); // <= FOUND
206:
207: return returnData;
208: }
['217']
217: function executeTransaction(
218: address target,
219: uint256 value,
220: string calldata signature,
221: bytes calldata data,
222: uint256 eta
223: ) public payable returns (bytes memory) {
224: require(msg.sender == admin, "Timelock::executeTransaction: Call must come from admin.");
225:
226: bytes32 txHash = keccak256(abi.encode(target, value, signature, data, eta));
227: require(queuedTransactions[txHash], "Timelock::executeTransaction: Transaction hasn't been queued.");
228: require(getBlockTimestamp() >= eta, "Timelock::executeTransaction: Transaction hasn't surpassed time lock.");
229: require(getBlockTimestamp() <= eta + GRACE_PERIOD(), "Timelock::executeTransaction: Transaction is stale.");
230:
231: delete (queuedTransactions[txHash]);
232:
233: bytes memory callData;
234:
235: if (bytes(signature).length == 0) {
236: callData = data;
237: } else {
238: callData = abi.encodePacked(bytes4(keccak256(bytes(signature))), data);
239: }
240:
241:
242: (bool success, bytes memory returnData) = target.call{ value: value }(callData); // <= FOUND
243: require(success, "Timelock::executeTransaction: Transaction execution reverted.");
244:
245: emit ExecuteTransaction(txHash, target, value, signature, data, eta); // <= FOUND
246:
247: return returnData;
248: }
Num of instances: 3
Click to show findings
['22']
22:
23: uint public constant MIN_VOTING_PERIOD = 20 * 60 * 3; // <= FOUND
['25']
25:
26: uint public constant MAX_VOTING_PERIOD = 20 * 60 * 24 * 14; // <= FOUND
['31']
31:
32: uint public constant MAX_VOTING_DELAY = 20 * 60 * 24 * 7; // <= FOUND
In struct definitions, using redundant prefixes for attributes is unnecessary. For instance, in a struct named Employee, attributes like employeeName, employeeID, and employeeEmail can be simplified to name, ID, and email respectively, since they are already inherently associated with Employee. By removing these repetitive prefixes, the code becomes more concise and easier to read, maintaining its contextual clarity.
Num of instances: 3
Click to show findings
['116']
116: struct Proposal {
117:
118: uint id;
119:
120: address proposer;
121:
122: uint eta;
123:
124: address[] targets;
125:
126: uint[] values;
127:
128: string[] signatures;
129:
130: bytes[] calldatas;
131:
132: uint startBlock;
133:
134: uint endBlock;
135:
136: uint forVotes;
137:
138: uint againstVotes;
139:
140: uint abstainVotes;
141:
142: bool canceled;
143:
144: bool executed;
145:
146: mapping(address => Receipt) receipts;
147:
148: uint8 proposalType; // <= FOUND
149: }
['193']
193: struct ProposalConfig {
194:
195: uint256 votingDelay;
196:
197: uint256 votingPeriod;
198:
199: uint256 proposalThreshold; // <= FOUND
200: }
['29']
29: struct Proposal {
30:
31: uint256 id;
32:
33: uint256 eta;
34:
35: address[] targets;
36:
37: uint256[] values;
38:
39: string[] signatures;
40:
41: bytes[] calldatas;
42:
43: bool canceled;
44:
45: bool executed;
46:
47: uint8 proposalType; // <= FOUND
48: }
Contracts should expose all public and external functions through interfaces. This practice ensures a clear and consistent definition of how the contract can be interacted with, promoting better transparency and integration.
Num of instances: 104
Click to show findings
['29']
29: function accessControlManager() external view returns (IAccessControlManagerV5)
['93']
93: function setAccessControlManager(address accessControlManager_) external onlyOwner
['55']
55: function accessControlManager() external view returns (IAccessControlManagerV8)
['48']
48: function setMaxDailyReceiveLimit(uint256 limit_) external onlyOwner
['57']
57: function pause() external onlyOwner
['65']
65: function unpause() external onlyOwner
['63']
63: function setMaxDailyLimit(uint16 chainId_, uint256 limit_) external
['57']
57: function pause() external
['65']
65: function unpause() external
['93']
93: function setAccessControlManager(address accessControlManager_) external onlyOwner
['172']
172: function queue(uint proposalId) external
['210']
210: function execute(uint proposalId) external
['233']
233: function cancel(uint proposalId) external
['263']
263: function getActions(
264: uint proposalId
265: )
266: external
267: view
268: returns (address[] memory targets, uint[] memory values, string[] memory signatures, bytes[] memory calldatas)
269:
['280']
280: function getReceipt(uint proposalId, address voter) external view returns (Receipt memory)
['319']
319: function castVote(uint proposalId, uint8 support) external
['329']
329: function castVoteWithReason(uint proposalId, uint8 support, string calldata reason) external
['337']
337: function castVoteBySig(uint proposalId, uint8 support, uint8 v, bytes32 r, bytes32 s) external
['382']
382: function _setGuardian(address newGuardian) external
['445']
445: function _initiate(address governorAlpha) external
['458']
458: function _setProposalMaxOperations(uint proposalMaxOperations_) external
['471']
471: function _setPendingAdmin(address newPendingAdmin) external
['489']
489: function _acceptAdmin() external
['172']
172: function queue(uint proposalId) external
['210']
210: function execute(uint proposalId) external
['233']
233: function cancel(uint proposalId) external
['319']
319: function castVote(uint proposalId, uint8 support) external
['329']
329: function castVoteWithReason(uint proposalId, uint8 support, string calldata reason) external
['337']
337: function castVoteBySig(uint proposalId, uint8 support, uint8 v, bytes32 r, bytes32 s) external
['395']
395: function _setVotingDelay(uint newVotingDelay) external
['411']
411: function _setVotingPeriod(uint newVotingPeriod) external
['428']
428: function _setProposalThreshold(uint newProposalThreshold) external
['445']
445: function _initiate(address governorAlpha) external
['43']
43: function initialize(address accessControlManager_) external initializer
['69']
69: function upsertSignature(string[] calldata signatures_, bool[] calldata active_) external onlyOwner
['94']
94: function transferBridgeOwnership(address newOwner_) external
['146']
146: function setSrcChainId(uint16 srcChainId_) external onlyOwner
['157']
157: function addTimelocks(ITimelock[] memory timelocks_) external onlyOwner
['178']
178: function execute(uint256 proposalId_) external nonReentrant
['212']
212: function cancel(uint256 proposalId_) external
['94']
94: function estimateFees(
95: uint16 remoteChainId_,
96: bytes calldata payload_,
97: bytes calldata adapterParams_
98: ) external view returns (uint256, uint256)
['108']
108: function removeTrustedRemote(uint16 remoteChainId_) external
['124']
124: function execute(
125: uint16 remoteChainId_,
126: bytes calldata payload_,
127: bytes calldata adapterParams_
128: ) external payable whenNotPaused
['169']
169: function retryExecute(
170: uint256 pId_,
171: uint16 remoteChainId_,
172: bytes calldata payload_,
173: bytes calldata adapterParams_,
174: uint256 originalValue_
175: ) external payable whenNotPaused nonReentrant
['214']
214: function fallbackWithdraw(
215: address to_,
216: uint256 pId_,
217: uint16 remoteChainId_,
218: bytes calldata payload_,
219: bytes calldata adapterParams_,
220: uint256 originalValue_
221: ) external onlyOwner nonReentrant
['249']
249: function setTrustedRemoteAddress(uint16 remoteChainId_, bytes calldata newRemoteAddress_) external
['266']
266: function setConfig(uint16 version_, uint16 chainId_, uint256 configType_, bytes calldata config_) external
['276']
276: function setSendVersion(uint16 version_) external
['287']
287: function getConfig(uint16 version_, uint16 chainId_, uint256 configType_) external view returns (bytes memory)
['77']
77: function giveCallPermission(address contractAddress, string calldata functionSig, address accountToPermit) public
['91']
91: function revokeCallPermission(
92: address contractAddress,
93: string calldata functionSig,
94: address accountToRevoke
95: ) public
['109']
109: function isAllowedToCall(address account, string calldata functionSig) public view returns (bool)
['128']
128: function hasPermission(
129: address account,
130: address contractAddress,
131: string calldata functionSig
132: ) public view returns (bool)
['47']
47: function _setImplementation(address implementation_) public
['9']
9: function quorumVotes() public pure returns (uint)
['14']
14: function proposalThreshold() public pure returns (uint)
['19']
19: function proposalMaxOperations() public pure returns (uint)
['24']
24: function votingDelay() public pure returns (uint)
['29']
29: function votingPeriod() public pure returns (uint)
['143']
143: function propose(
144: address[] memory targets,
145: uint[] memory values,
146: string[] memory signatures,
147: bytes[] memory calldatas,
148: string memory description
149: ) public returns (uint)
['213']
213: function queue(uint proposalId) public
['235']
235: function execute(uint proposalId) public payable
['254']
254: function cancel(uint proposalId) public
['279']
279: function getActions(
280: uint proposalId
281: )
282: public
283: view
284: returns (address[] memory targets, uint[] memory values, string[] memory signatures, bytes[] memory calldatas)
285:
['290']
290: function getReceipt(uint proposalId, address voter) public view returns (Receipt memory)
['294']
294: function state(uint proposalId) public view returns (ProposalState)
['316']
316: function castVote(uint proposalId, bool support) public
['320']
320: function castVoteBySig(uint proposalId, bool support, uint8 v, bytes32 r, bytes32 s) public
['351']
351: function __acceptAdmin() public
['356']
356: function __abdicate() public
['361']
361: function __queueSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public
['366']
366: function __executeSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public
['9']
9: function quorumVotes() public pure returns (uint)
['14']
14: function proposalThreshold() public pure returns (uint)
['19']
19: function proposalMaxOperations() public pure returns (uint)
['24']
24: function votingDelay() public pure returns (uint)
['29']
29: function votingPeriod() public pure returns (uint)
['143']
143: function propose(
144: address[] memory targets,
145: uint[] memory values,
146: string[] memory signatures,
147: bytes[] memory calldatas,
148: string memory description
149: ) public returns (uint)
['213']
213: function queue(uint proposalId) public
['235']
235: function execute(uint proposalId) public payable
['254']
254: function cancel(uint proposalId) public
['294']
294: function state(uint proposalId) public view returns (ProposalState)
['316']
316: function castVote(uint proposalId, bool support) public
['320']
320: function castVoteBySig(uint proposalId, bool support, uint8 v, bytes32 r, bytes32 s) public
['111']
111: function initialize(
112: address xvsVault_,
113: ProposalConfig[] memory proposalConfigs_,
114: TimelockInterface[] memory timelocks,
115: address guardian_
116: ) public
['181']
181: function propose(
182: address[] memory targets,
183: uint[] memory values,
184: string[] memory signatures,
185: bytes[] memory calldatas,
186: string memory description,
187: ProposalType proposalType
188: ) public returns (uint)
['294']
294: function state(uint proposalId) public view returns (ProposalState)
['51']
51: function initialize(
52: address timelock_,
53: address xvsVault_,
54: uint votingPeriod_,
55: uint votingDelay_,
56: uint proposalThreshold_,
57: address guardian_
58: ) public
['143']
143: function propose(
144: address[] memory targets,
145: uint[] memory values,
146: string[] memory signatures,
147: bytes[] memory calldatas,
148: string memory description
149: ) public returns (uint)
['294']
294: function state(uint proposalId) public view returns (ProposalState)
['47']
47: function _setImplementation(address implementation_) public
['246']
246: function state(uint256 proposalId_) public view returns (ProposalState)
['86']
86: function setDelay(uint delay_) public
['127']
127: function acceptAdmin() public
['140']
140: function setPendingAdmin(address pendingAdmin_) public
['126']
126: function queueTransaction(
127: address target,
128: uint value,
129: string memory signature,
130: bytes memory data,
131: uint eta
132: ) public returns (bytes32)
['154']
154: function cancelTransaction(
155: address target,
156: uint value,
157: string memory signature,
158: bytes memory data,
159: uint eta
160: ) public
['177']
177: function executeTransaction(
178: address target,
179: uint value,
180: string memory signature,
181: bytes memory data,
182: uint eta
183: ) public payable returns (bytes memory)
['90']
90: function setDelay(uint256 delay_) public
['127']
127: function acceptAdmin() public
['140']
140: function setPendingAdmin(address pendingAdmin_) public
['159']
159: function queueTransaction(
160: address target,
161: uint256 value,
162: string calldata signature,
163: bytes calldata data,
164: uint256 eta
165: ) public returns (bytes32)
['190']
190: function cancelTransaction(
191: address target,
192: uint256 value,
193: string calldata signature,
194: bytes calldata data,
195: uint256 eta
196: ) public
['217']
217: function executeTransaction(
218: address target,
219: uint256 value,
220: string calldata signature,
221: bytes calldata data,
222: uint256 eta
223: ) public payable returns (bytes memory)
Consider using abi.encode as this pads data to 32 byte segments
Num of instances: 1
Click to show findings
['325']
325: bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash)); // <= FOUND
In Solidity, private state variables are strictly confined to the contract they are defined in and can't be accessed or modified by its derived contracts. While this offers strong encapsulation, it can limit contract extensibility and modification in inheritance chains. On the other hand, internal variables can be accessed and potentially overridden by child contracts, granting more flexibility in contract development and upgrades. Therefore, it's recommended to use private only when you explicitly want to prevent child contract access. Otherwise, prefer internal to maintain a balance between encapsulation and the flexibility offered by inheritance patterns in Solidity.
Num of instances: 2
Click to show findings
['14']
14: IAccessControlManagerV5 private _accessControlManager; // <= FOUND
['17']
17: IAccessControlManagerV8 private _accessControlManager; // <= FOUND
Taking 0 as a valid argument in Solidity without checks can lead to severe security issues. A historical example is the infamous 0x0 address bug where numerous tokens were lost. This happens because '0' can be interpreted as an uninitialized address, leading to transfers to the '0x0' address, effectively burning tokens. Moreover, 0 as a denominator in division operations would cause a runtime exception. It's also often indicative of a logical error in the caller's code. It's important to always validate input and handle edge cases like 0 appropriately. Use require() statements to enforce conditions and provide clear error messages to facilitate debugging and safer code.
Num of instances: 8
Click to show findings
['109']
109: function isAllowedToCall(address account, string calldata functionSig) public view returns (bool) { // <= FOUND
110: bytes32 role = keccak256(abi.encodePacked(msg.sender, functionSig));
111:
112: if (hasRole(role, account)) {
113: return true;
114: } else {
115: role = keccak256(abi.encodePacked(address(0), functionSig)); // <= FOUND
116: return hasRole(role, account);
117: }
118: }
['143']
143: function propose(
144: address[] memory targets,
145: uint[] memory values,
146: string[] memory signatures,
147: bytes[] memory calldatas,
148: string memory description
149: ) public returns (uint) {
150: require(
151: xvs.getPriorVotes(msg.sender, sub256(block.number, 1)) > proposalThreshold(),
152: "GovernorAlpha::propose: proposer votes below proposal threshold"
153: );
154: require(
155: targets.length == values.length &&
156: targets.length == signatures.length &&
157: targets.length == calldatas.length,
158: "GovernorAlpha::propose: proposal function information arity mismatch"
159: );
160: require(targets.length != 0, "GovernorAlpha::propose: must provide actions");
161: require(targets.length <= proposalMaxOperations(), "GovernorAlpha::propose: too many actions");
162:
163: uint latestProposalId = latestProposalIds[msg.sender];
164: if (latestProposalId != 0) {
165: ProposalState proposersLatestProposalState = state(latestProposalId);
166: require(
167: proposersLatestProposalState != ProposalState.Active,
168: "GovernorAlpha::propose: found an already active proposal"
169: );
170: require(
171: proposersLatestProposalState != ProposalState.Pending,
172: "GovernorAlpha::propose: found an already pending proposal"
173: );
174: }
175:
176: uint startBlock = add256(block.number, votingDelay());
177: uint endBlock = add256(startBlock, votingPeriod());
178:
179: proposalCount++;
180: Proposal memory newProposal = Proposal({
181: id: proposalCount, // <= FOUND
182: proposer: msg.sender,
183: eta: 0,
184: targets: targets,
185: values: values,
186: signatures: signatures,
187: calldatas: calldatas,
188: startBlock: startBlock,
189: endBlock: endBlock,
190: forVotes: 0,
191: againstVotes: 0,
192: canceled: false,
193: executed: false
194: });
195:
196: proposals[newProposal.id] = newProposal;
197: latestProposalIds[newProposal.proposer] = newProposal.id;
198:
199: emit ProposalCreated(
200: newProposal.id,
201: msg.sender,
202: targets,
203: values,
204: signatures,
205: calldatas,
206: startBlock,
207: endBlock,
208: description
209: );
210: return newProposal.id;
211: }
['143']
143: function propose(
144: address[] memory targets,
145: uint[] memory values,
146: string[] memory signatures,
147: bytes[] memory calldatas,
148: string memory description
149: ) public returns (uint) {
150: require(
151: xvs.getPriorVotes(msg.sender, sub256(block.number, 1)) > proposalThreshold(),
152: "GovernorAlpha::propose: proposer votes below proposal threshold"
153: );
154: require(
155: targets.length == values.length &&
156: targets.length == signatures.length &&
157: targets.length == calldatas.length,
158: "GovernorAlpha::propose: proposal function information arity mismatch"
159: );
160: require(targets.length != 0, "GovernorAlpha::propose: must provide actions");
161: require(targets.length <= proposalMaxOperations(), "GovernorAlpha::propose: too many actions");
162:
163: uint latestProposalId = latestProposalIds[msg.sender];
164: if (latestProposalId != 0) {
165: ProposalState proposersLatestProposalState = state(latestProposalId);
166: require(
167: proposersLatestProposalState != ProposalState.Active,
168: "GovernorAlpha::propose: found an already active proposal"
169: );
170: require(
171: proposersLatestProposalState != ProposalState.Pending,
172: "GovernorAlpha::propose: found an already pending proposal"
173: );
174: }
175:
176: uint startBlock = add256(block.number, votingDelay());
177: uint endBlock = add256(startBlock, votingPeriod());
178:
179: proposalCount++;
180: Proposal memory newProposal = Proposal({
181: id: proposalCount, // <= FOUND
182: proposer: msg.sender,
183: eta: 0,
184: targets: targets,
185: values: values,
186: signatures: signatures,
187: calldatas: calldatas,
188: startBlock: startBlock,
189: endBlock: endBlock,
190: forVotes: 0,
191: againstVotes: 0,
192: canceled: false,
193: executed: false
194: });
195:
196: proposals[newProposal.id] = newProposal;
197: latestProposalIds[newProposal.proposer] = newProposal.id;
198:
199: emit ProposalCreated(
200: newProposal.id,
201: msg.sender,
202: targets,
203: values,
204: signatures,
205: calldatas,
206: startBlock,
207: endBlock,
208: description
209: );
210: return newProposal.id;
211: }
['361']
361: function __queueSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public { // <= FOUND
362: require(msg.sender == guardian, "GovernorAlpha::__queueSetTimelockPendingAdmin: sender must be gov guardian");
363: timelock.queueTransaction(address(timelock), 0, "setPendingAdmin(address)", abi.encode(newPendingAdmin), eta); // <= FOUND
364: }
['366']
366: function __executeSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public { // <= FOUND
367: require(msg.sender == guardian, "GovernorAlpha::__executeSetTimelockPendingAdmin: sender must be gov guardian");
368: timelock.executeTransaction(address(timelock), 0, "setPendingAdmin(address)", abi.encode(newPendingAdmin), eta); // <= FOUND
369: }
['181']
181: function propose(
182: address[] memory targets,
183: uint[] memory values,
184: string[] memory signatures,
185: bytes[] memory calldatas,
186: string memory description,
187: ProposalType proposalType
188: ) public returns (uint) {
189:
190: require(initialProposalId != 0, "GovernorBravo::propose: Governor Bravo not active");
191: require(
192: xvsVault.getPriorVotes(msg.sender, sub256(block.number, 1)) >=
193: proposalConfigs[uint8(proposalType)].proposalThreshold,
194: "GovernorBravo::propose: proposer votes below proposal threshold"
195: );
196: require(
197: targets.length == values.length &&
198: targets.length == signatures.length &&
199: targets.length == calldatas.length,
200: "GovernorBravo::propose: proposal function information arity mismatch"
201: );
202: require(targets.length != 0, "GovernorBravo::propose: must provide actions");
203: require(targets.length <= proposalMaxOperations, "GovernorBravo::propose: too many actions");
204:
205: uint latestProposalId = latestProposalIds[msg.sender];
206: if (latestProposalId != 0) {
207: ProposalState proposersLatestProposalState = state(latestProposalId);
208: require(
209: proposersLatestProposalState != ProposalState.Active,
210: "GovernorBravo::propose: one live proposal per proposer, found an already active proposal"
211: );
212: require(
213: proposersLatestProposalState != ProposalState.Pending,
214: "GovernorBravo::propose: one live proposal per proposer, found an already pending proposal"
215: );
216: }
217:
218: uint startBlock = add256(block.number, proposalConfigs[uint8(proposalType)].votingDelay);
219: uint endBlock = add256(startBlock, proposalConfigs[uint8(proposalType)].votingPeriod);
220:
221: proposalCount++;
222: Proposal memory newProposal = Proposal({
223: id: proposalCount, // <= FOUND
224: proposer: msg.sender,
225: eta: 0,
226: targets: targets,
227: values: values,
228: signatures: signatures,
229: calldatas: calldatas,
230: startBlock: startBlock,
231: endBlock: endBlock,
232: forVotes: 0,
233: againstVotes: 0,
234: abstainVotes: 0,
235: canceled: false,
236: executed: false,
237: proposalType: uint8(proposalType)
238: });
239:
240: proposals[newProposal.id] = newProposal;
241: latestProposalIds[newProposal.proposer] = newProposal.id;
242:
243: emit ProposalCreated(
244: newProposal.id,
245: msg.sender,
246: targets,
247: values,
248: signatures,
249: calldatas,
250: startBlock,
251: endBlock,
252: description,
253: uint8(proposalType)
254: );
255: return newProposal.id;
256: }
['95']
95: function propose(
96: address[] memory targets,
97: uint[] memory values,
98: string[] memory signatures,
99: bytes[] memory calldatas,
100: string memory description
101: ) public returns (uint) {
102:
103: require(initialProposalId != 0, "GovernorBravo::propose: Governor Bravo not active");
104: require(
105: xvsVault.getPriorVotes(msg.sender, sub256(block.number, 1)) > proposalThreshold,
106: "GovernorBravo::propose: proposer votes below proposal threshold"
107: );
108: require(
109: targets.length == values.length &&
110: targets.length == signatures.length &&
111: targets.length == calldatas.length,
112: "GovernorBravo::propose: proposal function information arity mismatch"
113: );
114: require(targets.length != 0, "GovernorBravo::propose: must provide actions");
115: require(targets.length <= proposalMaxOperations, "GovernorBravo::propose: too many actions");
116:
117: uint latestProposalId = latestProposalIds[msg.sender];
118: if (latestProposalId != 0) {
119: ProposalState proposersLatestProposalState = state(latestProposalId);
120: require(
121: proposersLatestProposalState != ProposalState.Active,
122: "GovernorBravo::propose: one live proposal per proposer, found an already active proposal"
123: );
124: require(
125: proposersLatestProposalState != ProposalState.Pending,
126: "GovernorBravo::propose: one live proposal per proposer, found an already pending proposal"
127: );
128: }
129:
130: uint startBlock = add256(block.number, votingDelay);
131: uint endBlock = add256(startBlock, votingPeriod);
132:
133: proposalCount++;
134: Proposal memory newProposal = Proposal({
135: id: proposalCount, // <= FOUND
136: proposer: msg.sender,
137: eta: 0,
138: targets: targets,
139: values: values,
140: signatures: signatures,
141: calldatas: calldatas,
142: startBlock: startBlock,
143: endBlock: endBlock,
144: forVotes: 0,
145: againstVotes: 0,
146: abstainVotes: 0,
147: canceled: false,
148: executed: false
149: });
150:
151: proposals[newProposal.id] = newProposal;
152: latestProposalIds[newProposal.proposer] = newProposal.id;
153:
154: emit ProposalCreated(
155: newProposal.id,
156: msg.sender,
157: targets,
158: values,
159: signatures,
160: calldatas,
161: startBlock,
162: endBlock,
163: description
164: );
165: return newProposal.id;
166: }
['296']
296: function _nonblockingLzReceive(uint16, bytes memory, uint64, bytes memory payload_) internal virtual override { // <= FOUND
297: (bytes memory payload, uint256 pId) = abi.decode(payload_, (bytes, uint256));
298: (
299: address[] memory targets,
300: uint256[] memory values,
301: string[] memory signatures,
302: bytes[] memory calldatas,
303: uint8 pType
304: ) = abi.decode(payload, (address[], uint256[], string[], bytes[], uint8));
305: require(proposals[pId].id == 0, "OmnichainGovernanceExecutor::_nonblockingLzReceive: duplicate proposal");
306: require(
307: targets.length == values.length &&
308: targets.length == signatures.length &&
309: targets.length == calldatas.length,
310: "OmnichainGovernanceExecutor::_nonblockingLzReceive: proposal function information arity mismatch"
311: );
312: require(
313: pType < uint8(type(ProposalType).max) + 1,
314: "OmnichainGovernanceExecutor::_nonblockingLzReceive: invalid proposal type"
315: );
316: _isEligibleToReceive(targets.length);
317:
318: Proposal memory newProposal = Proposal({
319: id: pId, // <= FOUND
320: eta: 0,
321: targets: targets,
322: values: values,
323: signatures: signatures,
324: calldatas: calldatas,
325: canceled: false,
326: executed: false,
327: proposalType: pType
328: });
329:
330: proposals[pId] = newProposal;
331: lastProposalReceived = pId;
332:
333: emit ProposalReceived(newProposal.id, targets, values, signatures, calldatas, pType);
334: _queue(pId);
335: }
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: 6
Click to show findings
['48']
48: function setAccessControlManager(address accessControlManager_) external onlyOwner { // <= FOUND
49: _setAccessControlManager(accessControlManager_);
50: }
['93']
93: function setAccessControlManager(address accessControlManager_) external onlyOwner { // <= FOUND
94: ensureNonzeroAddress(accessControlManager_);
95: emit NewAccessControlManager(accessControlManager, accessControlManager_);
96: accessControlManager = accessControlManager_;
97: }
['93']
93: function setAccessControlManager(address accessControlManager_) external onlyOwner // <= FOUND
['48']
48: function setMaxDailyReceiveLimit(uint256 limit_) external onlyOwner // <= FOUND
['93']
93: function setAccessControlManager(address accessControlManager_) external onlyOwner // <= FOUND
['146']
146: function setSrcChainId(uint16 srcChainId_) external onlyOwner // <= FOUND
It is preferable to skip operations on an array index when a condition is not met rather than reverting the whole transaction as reverting can introduce the possiblity of malicous actors purposefully introducing array objects which fail conditional checks within for/while loops so group operations fail. As such it is recommended to simply skip such array indices over reverting unless there is a valid security or logic reason behind not doing so.
Num of instances: 1
Click to show findings
['136']
136: for (uint256 i; i < arrLength; ++i) { // <= FOUND
137: require( // <= FOUND
138: proposalConfigs_[i].votingPeriod >= MIN_VOTING_PERIOD,
139: "GovernorBravo::initialize: invalid min voting period"
140: );
141: require( // <= FOUND
142: proposalConfigs_[i].votingPeriod <= MAX_VOTING_PERIOD,
143: "GovernorBravo::initialize: invalid max voting period"
144: );
145: require( // <= FOUND
146: proposalConfigs_[i].votingDelay >= MIN_VOTING_DELAY,
147: "GovernorBravo::initialize: invalid min voting delay"
148: );
149: require( // <= FOUND
150: proposalConfigs_[i].votingDelay <= MAX_VOTING_DELAY,
151: "GovernorBravo::initialize: invalid max voting delay"
152: );
153: require( // <= FOUND
154: proposalConfigs_[i].proposalThreshold >= MIN_PROPOSAL_THRESHOLD,
155: "GovernorBravo::initialize: invalid min proposal threshold"
156: );
157: require( // <= FOUND
158: proposalConfigs_[i].proposalThreshold <= MAX_PROPOSAL_THRESHOLD,
159: "GovernorBravo::initialize: invalid max proposal threshold"
160: );
161: require(address(timelocks[i]) != address(0), "GovernorBravo::initialize:invalid timelock address"); // <= FOUND
162:
163: proposalConfigs[i] = proposalConfigs_[i];
164: proposalTimelocks[i] = timelocks[i];
165: }
Num of instances: 1
It is general standard to declare interfaces on files separate from regular contract declarations
Num of instances: 2
Click to show findings
['391']
391: interface TimelockInterface // <= FOUND
['391']
391: interface TimelockInterface // <= FOUND
[NonCritical-16] Events regarding state variable changes should emit the previous state variable value
Modify such events to contain the previous value of the state variable as demonstrated in the example below
Num of instances: 4
Click to show findings
['13']
13: event NewAdmin(address indexed newAdmin);
['17']
17: event NewPendingAdmin(address indexed newPendingAdmin);
['19']
19: event NewDelay(uint indexed newDelay);
['30']
30: event FunctionRegistryChanged(string indexed signature, bool active);
[NonCritical-17] In functions which accept an address as a parameter, there should be a zero address check to prevent bugs
In smart contract development, especially with Solidity, it's crucial to validate inputs to functions. When a function accepts an Ethereum address as a parameter, implementing a zero address check (i.e., ensuring the address is not 0x0) is a best practice to prevent potential bugs and vulnerabilities. The zero address (0x0) is a default value and generally indicates an uninitialized or invalid state. Passing the zero address to certain functions can lead to unintended behaviors, like funds getting locked permanently or transactions failing silently. By checking for and rejecting the zero address, developers can ensure that the function operates as intended and interacts only with valid Ethereum addresses. This check enhances the contract's robustness and security.
Num of instances: 39
Click to show findings
['32']
32: function __AccessControlled_init(address accessControlManager_) internal onlyInitializing
['37']
37: function __AccessControlled_init_unchained(address accessControlManager_) internal onlyInitializing
['93']
93: function setAccessControlManager(address accessControlManager_) external onlyOwner
['77']
77: function giveCallPermission(address contractAddress, string calldata functionSig, address accountToPermit) public
['91']
91: function revokeCallPermission(
92: address contractAddress,
93: string calldata functionSig,
94: address accountToRevoke
95: ) public
['109']
109: function isAllowedToCall(address account, string calldata functionSig) public view returns (bool)
['128']
128: function hasPermission(
129: address account,
130: address contractAddress,
131: string calldata functionSig
132: ) public view returns (bool)
['93']
93: function setAccessControlManager(address accessControlManager_) external onlyOwner
['66']
66: function delegateTo(address callee, bytes memory data) internal
['143']
143: function propose(
144: address[] memory targets,
145: uint[] memory values,
146: string[] memory signatures,
147: bytes[] memory calldatas,
148: string memory description
149: ) public returns (uint)
['227']
227: function _queueOrRevert(address target, uint value, string memory signature, bytes memory data, uint eta) internal
['290']
290: function getReceipt(uint proposalId, address voter) public view returns (Receipt memory)
['331']
331: function _castVote(address voter, uint proposalId, bool support) internal
['361']
361: function __queueSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public
['366']
366: function __executeSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public
['143']
143: function propose(
144: address[] memory targets,
145: uint[] memory values,
146: string[] memory signatures,
147: bytes[] memory calldatas,
148: string memory description
149: ) public returns (uint)
['227']
227: function _queueOrRevert(address target, uint value, string memory signature, bytes memory data, uint eta) internal
['331']
331: function _castVote(address voter, uint proposalId, bool support) internal
['181']
181: function propose(
182: address[] memory targets,
183: uint[] memory values,
184: string[] memory signatures,
185: bytes[] memory calldatas,
186: string memory description,
187: ProposalType proposalType
188: ) public returns (uint)
['283']
283: function queueOrRevertInternal(
284: address target,
285: uint value,
286: string memory signature,
287: bytes memory data,
288: uint eta,
289: uint8 proposalType
290: ) internal
['280']
280: function getReceipt(uint proposalId, address voter) external view returns (Receipt memory)
['355']
355: function castVoteInternal(address voter, uint proposalId, uint8 support) internal returns (uint96)
['445']
445: function _initiate(address governorAlpha) external
['471']
471: function _setPendingAdmin(address newPendingAdmin) external
['143']
143: function propose(
144: address[] memory targets,
145: uint[] memory values,
146: string[] memory signatures,
147: bytes[] memory calldatas,
148: string memory description
149: ) public returns (uint)
['192']
192: function queueOrRevertInternal(
193: address target,
194: uint value,
195: string memory signature,
196: bytes memory data,
197: uint eta
198: ) internal
['355']
355: function castVoteInternal(address voter, uint proposalId, uint8 support) internal returns (uint96)
['445']
445: function _initiate(address governorAlpha) external
['66']
66: function delegateTo(address callee, bytes memory data) internal
['376']
376: function _queueOrRevertInternal(
377: address target_,
378: uint256 value_,
379: string memory signature_,
380: bytes memory data_,
381: uint256 eta_,
382: uint8 proposalType_
383: ) internal
['214']
214: function fallbackWithdraw(
215: address to_,
216: uint256 pId_,
217: uint16 remoteChainId_,
218: bytes calldata payload_,
219: bytes calldata adapterParams_,
220: uint256 originalValue_
221: ) external onlyOwner nonReentrant
['140']
140: function setPendingAdmin(address pendingAdmin_) public
['126']
126: function queueTransaction(
127: address target,
128: uint value,
129: string memory signature,
130: bytes memory data,
131: uint eta
132: ) public returns (bytes32)
['154']
154: function cancelTransaction(
155: address target,
156: uint value,
157: string memory signature,
158: bytes memory data,
159: uint eta
160: ) public
['177']
177: function executeTransaction(
178: address target,
179: uint value,
180: string memory signature,
181: bytes memory data,
182: uint eta
183: ) public payable returns (bytes memory)
['140']
140: function setPendingAdmin(address pendingAdmin_) public
['159']
159: function queueTransaction(
160: address target,
161: uint256 value,
162: string calldata signature,
163: bytes calldata data,
164: uint256 eta
165: ) public returns (bytes32)
['190']
190: function cancelTransaction(
191: address target,
192: uint256 value,
193: string calldata signature,
194: bytes calldata data,
195: uint256 eta
196: ) public
['217']
217: function executeTransaction(
218: address target,
219: uint256 value,
220: string calldata signature,
221: bytes calldata data,
222: uint256 eta
223: ) public payable returns (bytes memory)
Create a commented enum value to use in place of constant array indexes, this makes the code far easier to understand
Num of instances: 1
Click to show findings
['117']
117: require(address(proposalTimelocks[0]) == address(0), "GovernorBravo::initialize: cannot initialize twice"); // <= FOUND
In instances where a new variable is defined, there is no need to set it to it's default value.
Num of instances: 3
Click to show findings
['358']
358: guardian = address(0); // <= FOUND
['504']
504:
505: pendingAdmin = address(0); // <= FOUND
['504']
504: pendingAdmin = address(0); // <= FOUND
[NonCritical-20] Revert statements within external and public functions can be used to perform DOS attacks
In Solidity, 'revert' statements are used to undo changes and throw an exception when certain conditions are not met. However, in public and external functions, improper use of revert can be exploited for Denial of Service (DoS) attacks. An attacker can intentionally trigger these 'revert' conditions, causing legitimate transactions to consistently fail. For example, if a function relies on specific conditions from user input or contract state, an attacker could manipulate these to continually force reverts, blocking the function's execution. Therefore, it's crucial to design contract logic to handle exceptions properly and avoid scenarios where revert can be predictably triggered by malicious actors. This includes careful input validation and considering alternative design patterns that are less susceptible to such abuses.
Num of instances: 1
Click to show findings
['256']
246: function state(uint256 proposalId_) public view returns (ProposalState) {
247: Proposal storage proposal = proposals[proposalId_];
248: if (proposal.canceled) {
249: return ProposalState.Canceled;
250: } else if (proposal.executed) {
251: return ProposalState.Executed;
252: } else if (queued[proposalId_]) {
253:
254: return ProposalState.Queued;
255: } else {
256: revert InvalidProposalId(); // <= FOUND
257: }
258: }
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
[NonCritical-22] Functions which are either public or external should not have a preceding _ in their name
Remove the _ from the function name, ensure you also refactor where these functions are internally called
Num of instances: 15
Click to show findings
['382']
382: function _setGuardian(address newGuardian) external // <= FOUND
['445']
445: function _initiate(address governorAlpha) external // <= FOUND
['458']
458: function _setProposalMaxOperations(uint proposalMaxOperations_) external // <= FOUND
['471']
471: function _setPendingAdmin(address newPendingAdmin) external // <= FOUND
['489']
489: function _acceptAdmin() external // <= FOUND
['395']
395: function _setVotingDelay(uint newVotingDelay) external // <= FOUND
['411']
411: function _setVotingPeriod(uint newVotingPeriod) external // <= FOUND
['428']
428: function _setProposalThreshold(uint newProposalThreshold) external // <= FOUND
['445']
445: function _initiate(address governorAlpha) external // <= FOUND
['47']
47: function _setImplementation(address implementation_) public // <= FOUND
['351']
351: function __acceptAdmin() public // <= FOUND
['356']
356: function __abdicate() public // <= FOUND
['361']
361: function __queueSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public // <= FOUND
['366']
366: function __executeSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public // <= FOUND
['47']
47: function _setImplementation(address implementation_) public // <= FOUND
[NonCritical-23] Functions which are either private or internal should have a preceding _ in their name
Add a preceding underscore to the function name, take care to refactor where there functions are called
Num of instances: 25
Click to show findings
['66']
66: function delegateTo(address callee, bytes memory data) internal
['371']
371: function add256(uint256 a, uint256 b) internal pure returns (uint)
['377']
377: function sub256(uint256 a, uint256 b) internal pure returns (uint)
['382']
382: function getChainId() internal pure returns (uint)
['371']
371: function add256(uint256 a, uint256 b) internal pure returns (uint)
['377']
377: function sub256(uint256 a, uint256 b) internal pure returns (uint)
['382']
382: function getChainId() internal pure returns (uint)
['283']
283: function queueOrRevertInternal(
284: address target,
285: uint value,
286: string memory signature,
287: bytes memory data,
288: uint eta,
289: uint8 proposalType
290: ) internal
['355']
355: function castVoteInternal(address voter, uint proposalId, uint8 support) internal returns (uint96)
['521']
521: function getChainIdInternal() internal pure returns (uint)
['192']
192: function queueOrRevertInternal(
193: address target,
194: uint value,
195: string memory signature,
196: bytes memory data,
197: uint eta
198: ) internal
['355']
355: function castVoteInternal(address voter, uint proposalId, uint8 support) internal returns (uint96)
['521']
521: function getChainIdInternal() internal pure returns (uint)
['66']
66: function delegateTo(address callee, bytes memory data) internal
['26']
26: function add(uint256 a, uint256 b) internal pure returns (uint256)
['39']
39: function add(uint256 a, uint256 b, string memory errorMessage) internal pure returns (uint256)
['55']
55: function sub(uint256 a, uint256 b) internal pure returns (uint256)
['68']
68: function sub(uint256 a, uint256 b, string memory errorMessage) internal pure returns (uint256)
['84']
84: function mul(uint256 a, uint256 b) internal pure returns (uint256)
['109']
109: function div(uint256 a, uint256 b) internal pure returns (uint256)
['124']
124: function div(uint256 a, uint256 b, string memory errorMessage) internal pure returns (uint256)
['144']
144: function mod(uint256 a, uint256 b) internal pure returns (uint256)
['159']
159: function mod(uint256 a, uint256 b, string memory errorMessage) internal pure returns (uint256)
['210']
210: function getBlockTimestamp() internal view returns (uint)
['254']
254: function getBlockTimestamp() internal view returns (uint256)
Consider spreading these lines over multiple lines to aid in readability and the support of VIM users everywhere.
Num of instances: 51
Click to show findings
['8']
8: /// @notice The number of votes in support of a proposal required in order for a quorum to be reached and for a vote to succeed // <= FOUND
['9']
9: * It is included here for testing purposes because it has a different signature for the ProposalCreated event and Proposal struct // <= FOUND
['468']
468: * @dev Admin function to begin change of admin. The newPendingAdmin must call `_acceptAdmin` to finalize the transfer. // <= FOUND
['13']
13: * @notice OmnichainProposalSender contract builds upon the functionality of its parent contract , BaseOmnichainControllerSrc // <= FOUND
['37']
37: * @notice Specifies the allowed path for sending messages (remote chainId => remote app address + local app address) // <= FOUND
['87']
87: * @dev The estimated fees are the minimum required; it's recommended to increase the fees amount when sending a message. The unused amount will be refunded // <= FOUND
['89']
89: * @param payload_ The payload to be sent to the remote chain. It's computed as follows: payload = abi.encode(abi.encode(targets, values, signatures, calldatas), pId) // <= FOUND
['90']
90: * @param adapterParams_ The params used to specify the custom amount of gas required for the execution on the destination // <= FOUND
['118']
118: * @param payload_ The payload to be sent to the remote chain. It's computed as follows: payload = abi.encode(targets, values, signatures, calldatas, proposalType) // <= FOUND
['121']
121: * @custom:event Emits StorePayload with last stored payload proposal ID ,remote chain id , payload, adapter params , values and reason for failure // <= FOUND
['131']
131: // A zero value will result in a failed message; therefore, a positive value is required to send a message across the chain. // <= FOUND
['245']
245: * @param newRemoteAddress_ The address of the contract on the remote chain to receive messages sent by this contract // <= FOUND
['8']
8: * @notice Venus Governance latest on chain governance includes several new features including variable proposal routes and fine grained pause control. // <= FOUND
['9']
9: * Variable routes for proposals allows for governance paramaters such as voting threshold and timelocks to be customized based on the risk level and // <= FOUND
['10']
10: * impact of the proposal. Added granularity to the pause control mechanism allows governance to pause individual actions on specific markets, // <= FOUND
['13']
13: * The goal of **Governance** is to increase governance efficiency, while mitigating and eliminating malicious or erroneous proposals. // <= FOUND
['20']
20: * - XVSVault is the main staking contract for XVS. Users first stake their XVS in the vault and receive voting power proportional to their staked // <= FOUND
['30']
30: * `GovernanceBravoDelegate` uses the XVSVault to get restrict certain actions based on a user's voting power. The governance rules it inforces are: // <= FOUND
['32']
32: * - If a user's voting power drops below certain amount, anyone can cancel the the proposal. The governance guardian and proposal creator can also // <= FOUND
['37']
37: * Venus Governance allows for Venus Improvement Proposals (VIPs) to be categorized based on their impact and risk levels. This allows for optimizing proposals // <= FOUND
['38']
38: * execution to allow for things such as expediting interest rate changes and quickly updating risk parameters, while moving slower on other types of proposals // <= FOUND
['39']
39: * that can prevent a larger risk to the protocol and are not urgent. There are three different types of VIPs with different proposal paramters: // <= FOUND
['51']
51: * There is also a separate timelock executor contract for each route, which is used to dispatch the VIP for execution, giving even more control over the // <= FOUND
['56']
56: * After a VIP is proposed, voting is opened after the `votingDelay` has passed. For example, if `votingDelay = 0`, then voting will begin in the next block // <= FOUND
['57']
57: * after the proposal has been submitted. After the delay, the proposal state is `ACTIVE` and users can cast their vote `for`, `against`, or `abstain`, // <= FOUND
['58']
58: * weighted by their total voting power (tokens + delegated voting power). Abstaining from a voting allows for a vote to be cast and optionally include a // <= FOUND
['59']
59: * comment, without the incrementing for or against vote count. The total voting power for the user is obtained by calling XVSVault's `getPriorVotes`. // <= FOUND
['61']
61: * `GovernorBravoDelegate` also accepts [EIP-712](https://eips.ethereum.org/EIPS/eip-712) signatures for voting on proposals via the external function // <= FOUND
['66']
66: * A users voting power includes the amount of staked XVS the have staked as well as the votes delegate to them. Delegating is the process of a user loaning // <= FOUND
['70']
70: * The delegation of votes happens through the `XVSVault` contract by calling the `delegate` or `delegateBySig` functions. These same functions can revert // <= FOUND
['9']
9: * @dev This contract is a wrapper of OpenZeppelin AccessControl extending it in a way to standartize access control within Venus Smart Contract Ecosystem. // <= FOUND
['10']
10: * @notice Access control plays a crucial role in the Venus governance model. It is used to restrict functions so that they can only be called from one // <= FOUND
['13']
13: * The implementation of `AccessControlManager`(https://github.com/VenusProtocol/governance-contracts/blob/main/contracts/Governance/AccessControlManager.sol) // <= FOUND
['14']
14: * inherits the [Open Zeppelin AccessControl](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol) // <= FOUND
['19']
19: * Granular roles are built by hashing the contract address and its function signature. For example, given contract `Foo` with function `Foo.bar()` which // <= FOUND
['28']
28: * Admin roles allow for an address to call a function signature on any contract guarded by the `AccessControlManager`. This is particularly useful for // <= FOUND
['31']
31: * For Admin roles a null address is hashed in place of the contract address (`keccak256(0x0000000000000000000000000000000000000000,functionSignatureBar)`. // <= FOUND
['33']
33: * In the previous example, giving account B the admin role, account B will have permissions to call the `bar()` function on any contract that is guarded by // <= FOUND
['38']
38: * All restricted functions in Venus Protocol use a hook to ACM in order to check if the caller has the right permission to call the guarded function. // <= FOUND
['39']
39: * `AccessControlledV5` and `AccessControlledV8` abstract contract makes this integration easier. They call ACM's external method // <= FOUND
['40']
40: * `isAllowedToCall(address caller, string functionSig)`. Here is an example of how `setCollateralFactor` function in `Comptroller` is integrated with ACM: // <= FOUND
['45']
45: function setCollateralFactor(VToken vToken, uint256 newCollateralFactorMantissa, uint256 newLiquidationThresholdMantissa) external { // <= FOUND
['103']
103: * @dev Since restricted contracts using this function as a permission hook, we can get contracts address with msg.sender // <= FOUND
['122']
122: * @dev This function is used as a view function to check permissions rather than contract hook for access restriction check. // <= FOUND
['10']
10: * @dev This contract is included for archival and testing purposes as the ProposalCreated event has a different signature than the latest implementation. // <= FOUND
['86']
86: * @dev This contract is included for archival and testing purposes as the Proposal struct has a different shape than the latest implementation. // <= FOUND
['9']
9: * @notice This contract is helper between access control manager and actual contract. This contract further inherited by other contract (using solidity 0.5.16) // <= FOUND
['127']
127: // Revert if the last proposal is already sent in current block i.e multiple proposals cannot be sent within the same block.timestamp // <= FOUND
['14']
14: * @dev The owner of this contract controls LayerZero configuration. When used in production the owner will be OmnichainExecutor // <= FOUND
['288']
288: emit ReceivePayloadFailed(srcChainId_, srcAddress_, nonce_, reason); // Retrieve payload from the src side tx if needed to clear // <= FOUND
['12']
12: * @notice This contract is helper between access control manager and actual contract. This contract further inherited by other contract (using solidity 0.8.13) // <= FOUND
In Solidity, manipulating contract storage comes with significant gas costs. One can optimize gas usage by preventing unnecessary storage updates when the new value is the same as the existing one. If an existing value is the same as the new one, not reassigning it to the storage could potentially save substantial amounts of gas, notably 2900 gas for a 'Gsreset'. This saving may come at the expense of a cold storage load operation ('Gcoldsload'), which costs 2100 gas, or a warm storage access operation ('Gwarmaccess'), which costs 100 gas. Therefore, the gas efficiency of your contract can be significantly improved by adding a check that compares the new value with the current one before any storage update operation. If the values are the same, you can bypass the storage operation, thereby saving gas.
Num of instances: 4
Click to show findings
['48']
48: function setAccessControlManager(address accessControlManager_) external onlyOwner {
49: _setAccessControlManager(accessControlManager_);
50: }
['48']
48: function setMaxDailyReceiveLimit(uint256 limit_) external onlyOwner {
49: emit SetMaxDailyReceiveLimit(maxDailyReceiveLimit, limit_);
50: maxDailyReceiveLimit = limit_;
51: }
['93']
93: function setAccessControlManager(address accessControlManager_) external onlyOwner {
94: ensureNonzeroAddress(accessControlManager_);
95: emit NewAccessControlManager(accessControlManager, accessControlManager_);
96: accessControlManager = accessControlManager_;
97: }
['146']
146: function setSrcChainId(uint16 srcChainId_) external onlyOwner {
147: emit SetSrcChainId(srcChainId, srcChainId_);
148: srcChainId = srcChainId_;
149: }
In many cases only some functionality is used from an import. In such cases it makes more sense to use {} to specify what to import and thus save gas whilst improving readability
Num of instances: 8
Click to show findings
['4']
4: import "./IAccessControlManagerV5.sol";
['4']
4: import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
['5']
5: import "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol";
['4']
4: import "./IAccessControlManagerV8.sol";
['3']
3: import "@openzeppelin/contracts/access/AccessControl.sol";
['4']
4: import "./GovernorBravoInterfaces.sol";
['4']
4: import "@openzeppelin/contracts/access/IAccessControl.sol";
['3']
3: import "../Utils/SafeMath.sol";
Using outdated Solidity versions can lead to security risks and inefficiencies. It's recommended to adopt newer versions, ideally the latest, which as of now is 0.8.24. This ensures access to the latest bug fixes, features, and optimizations, particularly crucial for Layer 2 deployments. Regular updates to versions like 0.8.19 or later, up to 0.8.24, enhance contract security and performance.
Num of instances: 4
Click to show findings
[]
2: pragma solidity 0.5.16;
[]
2: pragma solidity 0.8.13;
[]
1: pragma solidity ^0.5.16;
[]
2: pragma experimental ABIEncoderV2;
Try to index as much as three variables in event declarations as this is more gas efficient when done on value type variables (uint, address etc) however not for bytes and string variables
Num of instances: 38
Click to show findings
['24']
24: event NewAccessControlManager(address oldAccessControlManager, address newAccessControlManager); // <= FOUND
['49']
49: event NewAccessControlManager(address indexed oldAccessControlManager, address indexed newAccessControlManager); // <= FOUND
['56']
56: event PermissionGranted(address account, address contractAddress, string functionSig); // <= FOUND
['59']
59: event PermissionRevoked(address account, address contractAddress, string functionSig); // <= FOUND
['45']
45: event SetMaxDailyLimit(uint16 indexed chainId, uint256 oldMaxLimit, uint256 newMaxLimit); // <= FOUND
['112']
112: event ProposalCreated( // <= FOUND
113: uint id,
114: address proposer,
115: address[] targets,
116: uint[] values,
117: string[] signatures,
118: bytes[] calldatas,
119: uint startBlock,
120: uint endBlock,
121: string description
122: );
['11']
11: event ProposalCreated( // <= FOUND
12: uint id,
13: address proposer,
14: address[] targets,
15: uint[] values,
16: string[] signatures,
17: bytes[] calldatas,
18: uint startBlock,
19: uint endBlock,
20: string description,
21: uint8 proposalType
22: );
['125']
125: event VoteCast(address voter, uint proposalId, bool support, uint votes); // <= FOUND
['32']
32: event VoteCast(address indexed voter, uint proposalId, uint8 support, uint votes, string reason); // <= FOUND
['128']
128: event ProposalCanceled(uint id); // <= FOUND
['117']
117: event ProposalCanceled(uint256 indexed id); // <= FOUND
['131']
131: event ProposalQueued(uint id, uint eta); // <= FOUND
['102']
102: event ProposalQueued(uint256 indexed id, uint256 eta); // <= FOUND
['134']
134: event ProposalExecuted(uint id); // <= FOUND
['107']
107: event ProposalExecuted(uint256 indexed id); // <= FOUND
['44']
44: event VotingDelaySet(uint oldVotingDelay, uint newVotingDelay); // <= FOUND
['47']
47: event VotingPeriodSet(uint oldVotingPeriod, uint newVotingPeriod); // <= FOUND
['50']
50: event NewImplementation(address oldImplementation, address newImplementation); // <= FOUND
['53']
53: event ProposalThresholdSet(uint oldProposalThreshold, uint newProposalThreshold); // <= FOUND
['56']
56: event NewPendingAdmin(address oldPendingAdmin, address newPendingAdmin); // <= FOUND
['17']
17: event NewPendingAdmin(address indexed newPendingAdmin); // <= FOUND
['59']
59: event NewAdmin(address oldAdmin, address newAdmin); // <= FOUND
['13']
13: event NewAdmin(address indexed newAdmin); // <= FOUND
['14']
14: event NewAdmin(address indexed oldAdmin, address indexed newAdmin); // <= FOUND
['62']
62: event NewGuardian(address oldGuardian, address newGuardian); // <= FOUND
['65']
65: event ProposalMaxOperationsUpdated(uint oldMaxOperations, uint newMaxOperations); // <= FOUND
['90']
90: event ProposalReceived( // <= FOUND
91: uint256 indexed proposalId,
92: address[] targets,
93: uint256[] values,
94: string[] signatures,
95: bytes[] calldatas,
96: uint8 proposalType
97: );
['122']
122: event TimelockAdded(uint8 routeType, address indexed oldTimelock, address indexed newTimelock); // <= FOUND
['54']
54: event ExecuteRemoteProposal(uint16 indexed remoteChainId, uint256 proposalId, bytes payload); // <= FOUND
['64']
64: event StorePayload( // <= FOUND
65: uint256 indexed proposalId,
66: uint16 indexed remoteChainId,
67: bytes payload,
68: bytes adapterParams,
69: uint256 value,
70: bytes reason
71: );
['75']
75: event FallbackWithdraw(address indexed receiver, uint256 value); // <= FOUND
['22']
22: event CancelTransaction( // <= FOUND
23: bytes32 indexed txHash,
24: address indexed target,
25: uint value,
26: string signature,
27: bytes data,
28: uint eta
29: );
['23']
23: event CancelTransaction( // <= FOUND
24: bytes32 indexed txHash,
25: address indexed target,
26: uint256 value,
27: string signature,
28: bytes data,
29: uint256 eta
30: );
['32']
32: event ExecuteTransaction( // <= FOUND
33: bytes32 indexed txHash,
34: address indexed target,
35: uint value,
36: string signature,
37: bytes data,
38: uint eta
39: );
['33']
33: event ExecuteTransaction( // <= FOUND
34: bytes32 indexed txHash,
35: address indexed target,
36: uint256 value,
37: string signature,
38: bytes data,
39: uint256 eta
40: );
['42']
42: event QueueTransaction( // <= FOUND
43: bytes32 indexed txHash,
44: address indexed target,
45: uint value,
46: string signature,
47: bytes data,
48: uint eta
49: );
['43']
43: event QueueTransaction( // <= FOUND
44: bytes32 indexed txHash,
45: address indexed target,
46: uint256 value,
47: string signature,
48: bytes data,
49: uint256 eta
50: );
['36']
36: event SetMaxDailyReceiveLimit(uint256 oldMaxLimit, uint256 newMaxLimit); // <= FOUND
This is the recommendation made by OpenZeppelin and allows plenty of room for updatability
Num of instances: 1
Instead of using uint to declare uint258, explicitly define uint258 to ensure there is no confusion
Num of instances: 70
Click to show findings
['17']
12: constructor(
13: address timelock_,
14: address xvsVault_,
15: address admin_,
16: address implementation_,
17: uint votingPeriod_, // <= FOUND
18: uint votingDelay_, // <= FOUND
19: uint proposalThreshold_, // <= FOUND
20: address guardian_
21: ) public {
['44']
43:
44: uint public proposalCount; // <= FOUND
['48']
47:
48: uint id; // <= FOUND
['52']
51:
52: uint eta; // <= FOUND
['62']
61:
62: uint startBlock; // <= FOUND
['64']
63:
64: uint endBlock; // <= FOUND
['66']
65:
66: uint forVotes; // <= FOUND
['68']
67:
68: uint againstVotes; // <= FOUND
['114']
112:
113: event ProposalCreated(
114: uint id, // <= FOUND
115: address proposer,
116: address[] targets,
117: uint[] values,
118: string[] signatures,
119: bytes[] calldatas,
120: uint startBlock, // <= FOUND
121: uint endBlock, // <= FOUND
122: string description
123: );
['126']
125:
126: event VoteCast(address voter, uint proposalId, bool support, uint votes); // <= FOUND
['132']
131:
132: event ProposalQueued(uint id, uint eta); // <= FOUND
['163']
163: uint latestProposalId = latestProposalIds[msg.sender]; // <= FOUND
['176']
176: uint startBlock = add256(block.number, votingDelay()); // <= FOUND
['177']
177: uint endBlock = add256(startBlock, votingPeriod()); // <= FOUND
['219']
219: uint eta = add256(block.timestamp, timelock.delay()); // <= FOUND
['227']
227: function _queueOrRevert(address target, uint value, string memory signature, bytes memory data, uint eta) internal { // <= FOUND
['280']
279: function getActions(
280: uint proposalId // <= FOUND
281: )
282: public
283: view
284: returns (address[] memory targets, uint[] memory values, string[] memory signatures, bytes[] memory calldatas)
285: {
['331']
331: function _castVote(address voter, uint proposalId, bool support) internal { // <= FOUND
['361']
361: function __queueSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public { // <= FOUND
['366']
366: function __executeSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public { // <= FOUND
['372']
372: uint c = a + b; // <= FOUND
['383']
383: uint chainId; // <= FOUND
['402']
400: function queueTransaction(
401: address target,
402: uint value, // <= FOUND
403: string calldata signature,
404: bytes calldata data,
405: uint eta // <= FOUND
406: ) external returns (bytes32);
['410']
408: function cancelTransaction(
409: address target,
410: uint value, // <= FOUND
411: string calldata signature,
412: bytes calldata data,
413: uint eta // <= FOUND
414: ) external;
['418']
416: function executeTransaction(
417: address target,
418: uint value, // <= FOUND
419: string calldata signature,
420: bytes calldata data,
421: uint eta // <= FOUND
422: ) external payable returns (bytes memory);
['426']
426: function getPriorVotes(address account, uint blockNumber) external view returns (uint96); // <= FOUND
['17']
16:
17: uint public constant MIN_PROPOSAL_THRESHOLD = 150000e18; // <= FOUND
['20']
19:
20: uint public constant MAX_PROPOSAL_THRESHOLD = 300000e18; // <= FOUND
['23']
22:
23: uint public constant MIN_VOTING_PERIOD = 20 * 60 * 3; // <= FOUND
['26']
25:
26: uint public constant MAX_VOTING_PERIOD = 20 * 60 * 24 * 14; // <= FOUND
['29']
28:
29: uint public constant MIN_VOTING_DELAY = 1; // <= FOUND
['32']
31:
32: uint public constant MAX_VOTING_DELAY = 20 * 60 * 24 * 7; // <= FOUND
['35']
34:
35: uint public constant quorumVotes = 600000e18; // <= FOUND
['218']
218: uint startBlock = add256(block.number, proposalConfigs[uint8(proposalType)].votingDelay); // <= FOUND
['219']
219: uint endBlock = add256(startBlock, proposalConfigs[uint8(proposalType)].votingPeriod); // <= FOUND
['268']
268: uint eta = add256(block.timestamp, proposalTimelocks[uint8(proposal.proposalType)].delay()); // <= FOUND
['285']
283: function queueOrRevertInternal(
284: address target,
285: uint value, // <= FOUND
286: string memory signature,
287: bytes memory data,
288: uint eta, // <= FOUND
289: uint8 proposalType
290: ) internal {
['269']
263:
268: function getActions(
269: uint proposalId // <= FOUND
270: )
271: external
272: view
273: returns (address[] memory targets, uint[] memory values, string[] memory signatures, bytes[] memory calldatas)
274: {
['362']
355:
362: function castVoteInternal(address voter, uint proposalId, uint8 support) internal returns (uint96) { // <= FOUND
['460']
460: uint oldProposalMaxOperations = proposalMaxOperations; // <= FOUND
['62']
51:
59: function initialize(
60: address timelock_,
61: address xvsVault_,
62: uint votingPeriod_, // <= FOUND
63: uint votingDelay_, // <= FOUND
64: uint proposalThreshold_, // <= FOUND
65: address guardian_
66: ) public {
['130']
130: uint startBlock = add256(block.number, votingDelay); // <= FOUND
['131']
131: uint endBlock = add256(startBlock, votingPeriod); // <= FOUND
['194']
192: function queueOrRevertInternal(
193: address target,
194: uint value, // <= FOUND
195: string memory signature,
196: bytes memory data,
197: uint eta // <= FOUND
198: ) internal {
['401']
401: uint oldVotingDelay = votingDelay; // <= FOUND
['417']
417: uint oldVotingPeriod = votingPeriod; // <= FOUND
['434']
434: uint oldProposalThreshold = proposalThreshold; // <= FOUND
['13']
11:
12: event ProposalCreated(
13: uint id, // <= FOUND
14: address proposer,
15: address[] targets,
16: uint[] values,
17: string[] signatures,
18: bytes[] calldatas,
19: uint startBlock, // <= FOUND
20: uint endBlock, // <= FOUND
21: string description,
22: uint8 proposalType
23: );
['38']
32:
38: event VoteCast(address indexed voter, uint proposalId, uint8 support, uint votes, string reason); // <= FOUND
['45']
44:
45: event VotingDelaySet(uint oldVotingDelay, uint newVotingDelay); // <= FOUND
['48']
47:
48: event VotingPeriodSet(uint oldVotingPeriod, uint newVotingPeriod); // <= FOUND
['54']
53:
54: event ProposalThresholdSet(uint oldProposalThreshold, uint newProposalThreshold); // <= FOUND
['66']
65:
66: event ProposalMaxOperationsUpdated(uint oldMaxOperations, uint newMaxOperations); // <= FOUND
['91']
90:
91: uint public votingDelay; // <= FOUND
['94']
93:
94: uint public votingPeriod; // <= FOUND
['97']
96:
97: uint public proposalThreshold; // <= FOUND
['100']
99:
100: uint public initialProposalId; // <= FOUND
['141']
140:
141: uint abstainVotes; // <= FOUND
['173']
172:
173: uint public proposalMaxOperations; // <= FOUND
['26']
22:
23: event CancelTransaction(
24: bytes32 indexed txHash,
25: address indexed target,
26: uint value, // <= FOUND
27: string signature,
28: bytes data,
29: uint eta // <= FOUND
30: );
['36']
32:
33: event ExecuteTransaction(
34: bytes32 indexed txHash,
35: address indexed target,
36: uint value, // <= FOUND
37: string signature,
38: bytes data,
39: uint eta // <= FOUND
40: );
['46']
42:
43: event QueueTransaction(
44: bytes32 indexed txHash,
45: address indexed target,
46: uint value, // <= FOUND
47: string signature,
48: bytes data,
49: uint eta // <= FOUND
50: );
['53']
52:
53: uint public constant GRACE_PERIOD = 14 days; // <= FOUND
['56']
55:
56: uint public constant MINIMUM_DELAY = 1 hours; // <= FOUND
['59']
58:
59: uint public constant MAXIMUM_DELAY = 30 days; // <= FOUND
['68']
67:
68: uint public delay; // <= FOUND
['72']
72: constructor(address admin_, uint delay_) public { // <= FOUND
['137']
126:
135: function queueTransaction(
136: address target,
137: uint value, // <= FOUND
138: string memory signature,
139: bytes memory data,
140: uint eta // <= FOUND
141: ) public returns (bytes32) {
['164']
154:
162: function cancelTransaction(
163: address target,
164: uint value, // <= FOUND
165: string memory signature,
166: bytes memory data,
167: uint eta // <= FOUND
168: ) public {
['187']
177:
185: function executeTransaction(
186: address target,
187: uint value, // <= FOUND
188: string memory signature,
189: bytes memory data,
190: uint eta // <= FOUND
191: ) public payable returns (bytes memory) {
The following order should be used within contracts
constructor
receive function (if exists)
fallback function (if exists)
external
public
internal
private
Rearrange the contract functions and contructors to fit this ordering
Num of instances: 7
Click to show findings
['11']
11: contract GovernorBravoDelegatorV1 is GovernorBravoDelegatorStorage, GovernorBravoEventsV1 // <= FOUND
['4']
4: contract GovernorAlpha // <= FOUND
['4']
4: contract GovernorAlpha2 // <= FOUND
['73']
73: contract GovernorBravoDelegate is GovernorBravoDelegateStorageV2, GovernorBravoEvents // <= FOUND
['11']
11: contract GovernorBravoDelegateV1 is GovernorBravoDelegateStorageV1, GovernorBravoEventsV1 // <= FOUND
['11']
11: contract GovernorBravoDelegator is GovernorBravoDelegatorStorage, GovernorBravoEvents // <= FOUND
['15']
15: abstract contract AccessControlledV8 is Initializable, Ownable2StepUpgradeable // <= FOUND
Double type casting should be avoided in Solidity contracts to prevent unintended consequences and ensure accurate data representation. Performing multiple type casts in succession can lead to unexpected truncation, rounding errors, or loss of precision, potentially compromising the contract's functionality and reliability. Furthermore, double type casting can make the code less readable and harder to maintain, increasing the likelihood of errors and misunderstandings during development and debugging. To ensure precise and consistent data handling, developers should use appropriate data types and avoid unnecessary or excessive type casting, promoting a more robust and dependable contract execution.
Num of instances: 1
Click to show findings
['252']
252: ensureNonzeroAddress(address(uint160(bytes20(newRemoteAddress_)))); // <= FOUND
In Solidity, when msg.sender plays a crucial role in a function's logic, it's important for transparency and auditability that any events emitted by this function include msg.sender as a parameter. This practice enhances the traceability and accountability of transactions, allowing users and external observers to easily track who initiated a particular action. Including msg.sender in event logs helps in creating a clear and verifiable record of interactions with the contract, thereby increasing user trust and facilitating easier debugging and analysis of contract behavior. It's a key aspect of writing clear, transparent, and user-friendly smart contracts.
Num of instances: 14
Click to show findings
['48']
47: function _setImplementation(address implementation_) public {
48: require(msg.sender == admin, "GovernorBravoDelegator::_setImplementation: admin only"); // <= FOUND
49: require(
50: implementation_ != address(0),
51: "GovernorBravoDelegator::_setImplementation: invalid implementation address"
52: );
53:
54: address oldImplementation = implementation;
55: implementation = implementation_;
56:
57: emit NewImplementation(oldImplementation, implementation);
58: }
['48']
47: function _setImplementation(address implementation_) public {
48: require(msg.sender == admin, "GovernorBravoDelegator::_setImplementation: admin only"); // <= FOUND
49: require(
50: implementation_ != address(0),
51: "GovernorBravoDelegator::_setImplementation: invalid implementation address"
52: );
53:
54: address oldImplementation = implementation;
55: implementation = implementation_;
56:
57: emit NewImplementation(oldImplementation, implementation);
58: }
['260']
254: function cancel(uint proposalId) public {
255: ProposalState state = state(proposalId);
256: require(state != ProposalState.Executed, "GovernorAlpha::cancel: cannot cancel executed proposal");
257:
258: Proposal storage proposal = proposals[proposalId];
259: require(
260: msg.sender == guardian || // <= FOUND
261: xvs.getPriorVotes(proposal.proposer, sub256(block.number, 1)) < proposalThreshold(),
262: "GovernorAlpha::cancel: proposer above threshold"
263: );
264:
265: proposal.canceled = true;
266: for (uint i = 0; i < proposal.targets.length; i++) {
267: timelock.cancelTransaction(
268: proposal.targets[i],
269: proposal.values[i],
270: proposal.signatures[i],
271: proposal.calldatas[i],
272: proposal.eta
273: );
274: }
275:
276: emit ProposalCanceled(proposalId);
277: }
['260']
254: function cancel(uint proposalId) public {
255: ProposalState state = state(proposalId);
256: require(state != ProposalState.Executed, "GovernorAlpha::cancel: cannot cancel executed proposal");
257:
258: Proposal storage proposal = proposals[proposalId];
259: require(
260: msg.sender == guardian || // <= FOUND
261: xvs.getPriorVotes(proposal.proposer, sub256(block.number, 1)) < proposalThreshold(),
262: "GovernorAlpha::cancel: proposer above threshold"
263: );
264:
265: proposal.canceled = true;
266: for (uint i = 0; i < proposal.targets.length; i++) {
267: timelock.cancelTransaction(
268: proposal.targets[i],
269: proposal.values[i],
270: proposal.signatures[i],
271: proposal.calldatas[i],
272: proposal.eta
273: );
274: }
275:
276: emit ProposalCanceled(proposalId);
277: }
['332']
327: function cancel(uint proposalId) external {
328: require(state(proposalId) != ProposalState.Executed, "GovernorBravo::cancel: cannot cancel executed proposal");
329:
330: Proposal storage proposal = proposals[proposalId];
331: require(
332: msg.sender == guardian || // <= FOUND
333: msg.sender == proposal.proposer || // <= FOUND
334: xvsVault.getPriorVotes(proposal.proposer, sub256(block.number, 1)) <
335: proposalConfigs[proposal.proposalType].proposalThreshold,
336: "GovernorBravo::cancel: proposer above threshold"
337: );
338:
339: proposal.canceled = true;
340: for (uint i = 0; i < proposal.targets.length; i++) {
341: proposalTimelocks[proposal.proposalType].cancelTransaction(
342: proposal.targets[i],
343: proposal.values[i],
344: proposal.signatures[i],
345: proposal.calldatas[i],
346: proposal.eta
347: );
348: }
349:
350: emit ProposalCanceled(proposalId);
351: }
['238']
233: function cancel(uint proposalId) external {
234: require(state(proposalId) != ProposalState.Executed, "GovernorBravo::cancel: cannot cancel executed proposal");
235:
236: Proposal storage proposal = proposals[proposalId];
237: require(
238: msg.sender == guardian || // <= FOUND
239: msg.sender == proposal.proposer || // <= FOUND
240: xvsVault.getPriorVotes(proposal.proposer, sub256(block.number, 1)) < proposalThreshold,
241: "GovernorBravo::cancel: proposer above threshold"
242: );
243:
244: proposal.canceled = true;
245: for (uint i = 0; i < proposal.targets.length; i++) {
246: timelock.cancelTransaction(
247: proposal.targets[i],
248: proposal.values[i],
249: proposal.signatures[i],
250: proposal.calldatas[i],
251: proposal.eta
252: );
253: }
254:
255: emit ProposalCanceled(proposalId);
256: }
['383']
382: function _setGuardian(address newGuardian) external {
383: require(msg.sender == guardian || msg.sender == admin, "GovernorBravo::_setGuardian: admin or guardian only"); // <= FOUND
384: require(newGuardian != address(0), "GovernorBravo::_setGuardian: cannot live without a guardian");
385: address oldGuardian = guardian;
386: guardian = newGuardian;
387:
388: emit NewGuardian(oldGuardian, newGuardian);
389: }
['459']
458: function _setProposalMaxOperations(uint proposalMaxOperations_) external {
459: require(msg.sender == admin, "GovernorBravo::_setProposalMaxOperations: admin only"); // <= FOUND
460: uint oldProposalMaxOperations = proposalMaxOperations;
461: proposalMaxOperations = proposalMaxOperations_;
462:
463: emit ProposalMaxOperationsUpdated(oldProposalMaxOperations, proposalMaxOperations_);
464: }
['473']
471: function _setPendingAdmin(address newPendingAdmin) external {
472:
473: require(msg.sender == admin, "GovernorBravo:_setPendingAdmin: admin only"); // <= FOUND
474:
475:
476: address oldPendingAdmin = pendingAdmin;
477:
478:
479: pendingAdmin = newPendingAdmin;
480:
481:
482: emit NewPendingAdmin(oldPendingAdmin, newPendingAdmin);
483: }
['492']
489: function _acceptAdmin() external {
490:
491: require(
492: msg.sender == pendingAdmin && msg.sender != address(0), // <= FOUND
493: "GovernorBravo:_acceptAdmin: pending admin only"
494: );
495:
496:
497: address oldAdmin = admin;
498: address oldPendingAdmin = pendingAdmin;
499:
500:
501: admin = pendingAdmin;
502:
503:
504: pendingAdmin = address(0);
505:
506: emit NewAdmin(oldAdmin, admin);
507: emit NewPendingAdmin(oldPendingAdmin, pendingAdmin);
508: }
['396']
395: function _setVotingDelay(uint newVotingDelay) external {
396: require(msg.sender == admin, "GovernorBravo::_setVotingDelay: admin only"); // <= FOUND
397: require(
398: newVotingDelay >= MIN_VOTING_DELAY && newVotingDelay <= MAX_VOTING_DELAY,
399: "GovernorBravo::_setVotingDelay: invalid voting delay"
400: );
401: uint oldVotingDelay = votingDelay;
402: votingDelay = newVotingDelay;
403:
404: emit VotingDelaySet(oldVotingDelay, votingDelay);
405: }
['412']
411: function _setVotingPeriod(uint newVotingPeriod) external {
412: require(msg.sender == admin, "GovernorBravo::_setVotingPeriod: admin only"); // <= FOUND
413: require(
414: newVotingPeriod >= MIN_VOTING_PERIOD && newVotingPeriod <= MAX_VOTING_PERIOD,
415: "GovernorBravo::_setVotingPeriod: invalid voting period"
416: );
417: uint oldVotingPeriod = votingPeriod;
418: votingPeriod = newVotingPeriod;
419:
420: emit VotingPeriodSet(oldVotingPeriod, votingPeriod);
421: }
['429']
428: function _setProposalThreshold(uint newProposalThreshold) external {
429: require(msg.sender == admin, "GovernorBravo::_setProposalThreshold: admin only"); // <= FOUND
430: require(
431: newProposalThreshold >= MIN_PROPOSAL_THRESHOLD && newProposalThreshold <= MAX_PROPOSAL_THRESHOLD,
432: "GovernorBravo::_setProposalThreshold: invalid proposal threshold"
433: );
434: uint oldProposalThreshold = proposalThreshold;
435: proposalThreshold = newProposalThreshold;
436:
437: emit ProposalThresholdSet(oldProposalThreshold, proposalThreshold);
438: }
['218']
212: function cancel(uint256 proposalId_) external {
213: require(
214: state(proposalId_) == ProposalState.Queued,
215: "OmnichainGovernanceExecutor::cancel: proposal should be queued and not executed"
216: );
217: Proposal storage proposal = proposals[proposalId_];
218: require(msg.sender == GUARDIAN, "OmnichainGovernanceExecutor::cancel: sender must be guardian"); // <= FOUND
219:
220: proposal.canceled = true;
221: ITimelock timelock = proposalTimelocks[proposal.proposalType];
222: uint256 eta = proposal.eta;
223: uint256 length = proposal.targets.length;
224:
225: emit ProposalCanceled(proposalId_);
226:
227: for (uint256 i; i < length; ) {
228: timelock.cancelTransaction(
229: proposal.targets[i],
230: proposal.values[i],
231: proposal.signatures[i],
232: proposal.calldatas[i],
233: eta
234: );
235: unchecked {
236: ++i;
237: }
238: }
239: }
Amend the ordering of imports to import interfaces first followed by other imports
Num of instances: 6
Click to show findings
['7']
3:
5: pragma solidity 0.8.13;
6:
7: import { ReentrancyGuard } from "@openzeppelin/contracts/security/ReentrancyGuard.sol"; // <= FOUND
8: import { ILayerZeroEndpoint } from "@layerzerolabs/solidity-examples/contracts/lzApp/interfaces/ILayerZeroEndpoint.sol"; // <= FOUND
9: import { ensureNonzeroAddress } from "@venusprotocol/solidity-utilities/contracts/validators.sol"; // <= FOUND
10: import { BaseOmnichainControllerSrc } from "./BaseOmnichainControllerSrc.sol"; // <= FOUND
11:
21: contract OmnichainProposalSender is ReentrancyGuard, BaseOmnichainControllerSrc {
22:
25: uint256 public proposalCount;
26:
31: mapping(uint256 => bytes32) public storedExecutionHashes;
32:
36: ILayerZeroEndpoint public immutable LZ_ENDPOINT;
37:
41: mapping(uint16 => bytes) public trustedRemoteLookup;
42:
46: event SetTrustedRemoteAddress(uint16 indexed remoteChainId, bytes oldRemoteAddress, bytes newRemoteAddress);
47:
51: event TrustedRemoteRemoved(uint16 indexed chainId);
52:
56: event ExecuteRemoteProposal(uint16 indexed remoteChainId, uint256 proposalId, bytes payload);
57:
61: event ClearPayload(uint256 indexed proposalId, bytes32 executionHash);
62:
66: event StorePayload(
67: uint256 indexed proposalId,
68: uint16 indexed remoteChainId,
69: bytes payload,
70: bytes adapterParams,
71: uint256 value,
72: bytes reason
73: );
74:
77: event FallbackWithdraw(address indexed receiver, uint256 value);
78:
79: constructor(
80: ILayerZeroEndpoint lzEndpoint_,
81: address accessControlManager_
82: ) BaseOmnichainControllerSrc(accessControlManager_) {
83: ensureNonzeroAddress(address(lzEndpoint_));
84: LZ_ENDPOINT = lzEndpoint_;
85: }
86:
96: function estimateFees(
97: uint16 remoteChainId_,
98: bytes calldata payload_,
99: bytes calldata adapterParams_
100: ) external view returns (uint256, uint256) {
101: return LZ_ENDPOINT.estimateFees(remoteChainId_, address(this), payload_, false, adapterParams_);
102: }
103:
110: function removeTrustedRemote(uint16 remoteChainId_) external {
111: _ensureAllowed("removeTrustedRemote(uint16)");
112: delete trustedRemoteLookup[remoteChainId_];
113: emit TrustedRemoteRemoved(remoteChainId_);
['4']
2:
3: pragma solidity 0.8.13;
4: import "@openzeppelin/contracts/access/AccessControl.sol"; // <= FOUND
5: import "./IAccessControlManagerV8.sol"; // <= FOUND
6:
53: contract AccessControlManager is AccessControl, IAccessControlManagerV8 {
54:
57: event PermissionGranted(address account, address contractAddress, string functionSig);
58:
60: event PermissionRevoked(address account, address contractAddress, string functionSig);
61:
62: constructor() {
63:
64:
65: _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
66: }
67:
78: function giveCallPermission(address contractAddress, string calldata functionSig, address accountToPermit) public {
79: bytes32 role = keccak256(abi.encodePacked(contractAddress, functionSig));
80: grantRole(role, accountToPermit);
81: emit PermissionGranted(accountToPermit, contractAddress, functionSig);
82: }
83:
92: function revokeCallPermission(
93: address contractAddress,
94: string calldata functionSig,
95: address accountToRevoke
96: ) public {
97: bytes32 role = keccak256(abi.encodePacked(contractAddress, functionSig));
98: revokeRole(role, accountToRevoke);
99: emit PermissionRevoked(accountToRevoke, contractAddress, functionSig);
100: }
101:
110: function isAllowedToCall(address account, string calldata functionSig) public view returns (bool) {
111: bytes32 role = keccak256(abi.encodePacked(msg.sender, functionSig));
112:
113: if (hasRole(role, account)) {
114: return true;
115: } else {
116: role = keccak256(abi.encodePacked(address(0), functionSig));
117: return hasRole(role, account);
118: }
119: }
120:
129: function hasPermission(
130: address account,
131: address contractAddress,
132: string calldata functionSig
133: ) public view returns (bool) {
134: bytes32 role = keccak256(abi.encodePacked(contractAddress, functionSig));
135: return hasRole(role, account);
136: }
137: }
['5']
2:
3: pragma solidity 0.8.13;
4:
5: import { AccessControlledV8 } from "../Governance/AccessControlledV8.sol"; // <= FOUND
6: import { IOmnichainGovernanceExecutor } from "./interfaces/IOmnichainGovernanceExecutor.sol"; // <= FOUND
7:
17: contract OmnichainExecutorOwner is AccessControlledV8 {
18:
21: IOmnichainGovernanceExecutor public immutable OMNICHAIN_GOVERNANCE_EXECUTOR;
22:
26: mapping(bytes4 => string) public functionRegistry;
27:
31: event FunctionRegistryChanged(string indexed signature, bool active);
32:
34: constructor(address omnichainGovernanceExecutor_) {
35: require(omnichainGovernanceExecutor_ != address(0), "Address must not be zero");
36: OMNICHAIN_GOVERNANCE_EXECUTOR = IOmnichainGovernanceExecutor(omnichainGovernanceExecutor_);
37: _disableInitializers();
38: }
39:
44: function initialize(address accessControlManager_) external initializer {
45: require(accessControlManager_ != address(0), "Address must not be zero");
46: __AccessControlled_init(accessControlManager_);
47: }
48:
55: fallback(bytes calldata data_) external returns (bytes memory) {
56: string memory fun = functionRegistry[msg.sig];
57: require(bytes(fun).length != 0, "Function not found");
58: _checkAccessAllowed(fun);
59: (bool ok, bytes memory res) = address(OMNICHAIN_GOVERNANCE_EXECUTOR).call(data_);
60: require(ok, "call failed");
61: return res;
62: }
63:
70: function upsertSignature(string[] calldata signatures_, bool[] calldata active_) external onlyOwner {
71: uint256 signatureLength = signatures_.length;
72: require(signatureLength == active_.length, "Input arrays must have the same length");
73: for (uint256 i; i < signatureLength; ) {
74: bytes4 sigHash = bytes4(keccak256(bytes(signatures_[i])));
75: bytes memory signature = bytes(functionRegistry[sigHash]);
76: if (active_[i] && signature.length == 0) {
77: functionRegistry[sigHash] = signatures_[i];
78: emit FunctionRegistryChanged(signatures_[i], true);
79: } else if (!active_[i] && signature.length != 0) {
80: delete functionRegistry[sigHash];
81: emit FunctionRegistryChanged(signatures_[i], false);
82: }
83: unchecked {
84: ++i;
85: }
86: }
87: }
88:
95: function transferBridgeOwnership(address newOwner_) external {
['7']
3:
5: pragma solidity 0.8.13;
6:
7: import { Pausable } from "@openzeppelin/contracts/security/Pausable.sol"; // <= FOUND
8: import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; // <= FOUND
9: import { ensureNonzeroAddress } from "@venusprotocol/solidity-utilities/contracts/validators.sol"; // <= FOUND
10: import { IAccessControlManagerV8 } from "./../Governance/IAccessControlManagerV8.sol"; // <= FOUND
11:
19: contract BaseOmnichainControllerSrc is Ownable, Pausable {
20:
23: address public accessControlManager;
24:
28: mapping(uint16 => uint256) public chainIdToMaxDailyLimit;
29:
33: mapping(uint16 => uint256) public chainIdToLast24HourCommandsSent;
34:
38: mapping(uint16 => uint256) public chainIdToLast24HourWindowStart;
39:
42: mapping(uint16 => uint256) public chainIdToLastProposalSentTimestamp;
43:
47: event SetMaxDailyLimit(uint16 indexed chainId, uint256 oldMaxLimit, uint256 newMaxLimit);
48:
51: event NewAccessControlManager(address indexed oldAccessControlManager, address indexed newAccessControlManager);
52:
53: constructor(address accessControlManager_) {
54: ensureNonzeroAddress(accessControlManager_);
55: accessControlManager = accessControlManager_;
56: }
57:
65: function setMaxDailyLimit(uint16 chainId_, uint256 limit_) external {
66: _ensureAllowed("setMaxDailyLimit(uint16,uint256)");
67: emit SetMaxDailyLimit(chainId_, chainIdToMaxDailyLimit[chainId_], limit_);
68: chainIdToMaxDailyLimit[chainId_] = limit_;
69: }
70:
75: function pause() external {
76: _ensureAllowed("pause()");
77: _pause();
78: }
79:
84: function unpause() external {
85: _ensureAllowed("unpause()");
86: _unpause();
87: }
88:
95: function setAccessControlManager(address accessControlManager_) external onlyOwner {
96: ensureNonzeroAddress(accessControlManager_);
97: emit NewAccessControlManager(accessControlManager, accessControlManager_);
98: accessControlManager = accessControlManager_;
99: }
100:
104: function renounceOwnership() public override {}
105:
111: function _isEligibleToSend(uint16 dstChainId_, uint256 noOfCommands_) internal {
112:
113: uint256 currentBlockTimestamp = block.timestamp;
114: uint256 lastDayWindowStart = chainIdToLast24HourWindowStart[dstChainId_];
['5']
2:
3: pragma solidity 0.8.13;
4:
5: import { ReentrancyGuard } from "@openzeppelin/contracts/security/ReentrancyGuard.sol"; // <= FOUND
6: import { BytesLib } from "@layerzerolabs/solidity-examples/contracts/libraries/BytesLib.sol"; // <= FOUND
7: import { ExcessivelySafeCall } from "@layerzerolabs/solidity-examples/contracts/libraries/ExcessivelySafeCall.sol"; // <= FOUND
8: import { ensureNonzeroAddress } from "@venusprotocol/solidity-utilities/contracts/validators.sol"; // <= FOUND
9: import { BaseOmnichainControllerDest } from "./BaseOmnichainControllerDest.sol"; // <= FOUND
10: import { ITimelock } from "./interfaces/ITimelock.sol"; // <= FOUND
11:
20: contract OmnichainGovernanceExecutor is ReentrancyGuard, BaseOmnichainControllerDest {
21: using BytesLib for bytes;
22: using ExcessivelySafeCall for address;
23:
24: enum ProposalType {
25: NORMAL,
26: FASTTRACK,
27: CRITICAL
28: }
29:
30: struct Proposal {
31:
32: uint256 id;
33:
34: uint256 eta;
35:
36: address[] targets;
37:
38: uint256[] values;
39:
40: string[] signatures;
41:
42: bytes[] calldatas;
43:
44: bool canceled;
45:
46: bool executed;
47:
48: uint8 proposalType;
49: }
50:
52: enum ProposalState {
53: Canceled,
54: Queued,
55: Executed
56: }
57:
61: address public immutable GUARDIAN;
62:
66: uint16 public srcChainId;
67:
71: uint256 public lastProposalReceived;
72:
76: mapping(uint256 => Proposal) public proposals;
77:
81: mapping(uint256 => ITimelock) public proposalTimelocks;
82:
86: mapping(uint256 => bool) public queued;
['5']
2:
3: pragma solidity 0.8.13;
4:
5: import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; // <= FOUND
6: import "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol"; // <= FOUND
7:
8: import "./IAccessControlManagerV8.sol"; // <= FOUND
9:
16: abstract contract AccessControlledV8 is Initializable, Ownable2StepUpgradeable {
17:
18: IAccessControlManagerV8 private _accessControlManager;
19:
25: uint256[49] private __gap;
26:
28: event NewAccessControlManager(address oldAccessControlManager, address newAccessControlManager);
29:
31: error Unauthorized(address sender, address calledContract, string methodSignature);
32:
33: function __AccessControlled_init(address accessControlManager_) internal onlyInitializing {
34: __Ownable2Step_init();
35: __AccessControlled_init_unchained(accessControlManager_);
36: }
37:
38: function __AccessControlled_init_unchained(address accessControlManager_) internal onlyInitializing {
39: _setAccessControlManager(accessControlManager_);
40: }
41:
49: function setAccessControlManager(address accessControlManager_) external onlyOwner {
50: _setAccessControlManager(accessControlManager_);
51: }
52:
56: function accessControlManager() external view returns (IAccessControlManagerV8) {
57: return _accessControlManager;
58: }
59:
64: function _setAccessControlManager(address accessControlManager_) internal {
65: require(address(accessControlManager_) != address(0), "invalid acess control manager address");
66: address oldAccessControlManager = address(_accessControlManager);
67: _accessControlManager = IAccessControlManagerV8(accessControlManager_);
68: emit NewAccessControlManager(oldAccessControlManager, accessControlManager_);
69: }
70:
75: function _checkAccessAllowed(string memory signature) internal view {
76: bool isAllowedToCall = _accessControlManager.isAllowedToCall(msg.sender, signature);
77:
78: if (!isAllowedToCall) {
79: revert Unauthorized(msg.sender, address(this), signature);
80: }
81: }
82: }
83:
Using a single struct mapping in place of multiple defined mappings in a Solidity contract can lead to improved code organization, better readability, and easier maintainability. By consolidating related data into a single struct, developers can create a more cohesive data structure that logically groups together relevant pieces of information, thus reducing redundancy and clutter. This approach simplifies the codebase, making it easier to understand, navigate, and modify. Additionally, it can result in more efficient gas usage when accessing or updating multiple related data points simultaneously.
Num of instances: 8
Click to show findings
['26']
17: contract BaseOmnichainControllerSrc is Ownable, Pausable {
18:
21: address public accessControlManager;
22:
26: mapping(uint16 => uint256) public chainIdToMaxDailyLimit; // <= FOUND
27:
31: mapping(uint16 => uint256) public chainIdToLast24HourCommandsSent; // <= FOUND
32:
36: mapping(uint16 => uint256) public chainIdToLast24HourWindowStart; // <= FOUND
37:
40: mapping(uint16 => uint256) public chainIdToLastProposalSentTimestamp; // <= FOUND
41:
99: }
['53']
4: contract GovernorAlpha {
5:
6: string public constant name = "Venus Governor Alpha";
7:
24: TimelockInterface public timelock;
25:
27: XVSInterface public xvs;
28:
30: address public guardian;
31:
33: uint public proposalCount;
34:
41: enum ProposalState {
42: Pending,
43: Active,
44: Canceled,
45: Defeated,
46: Succeeded,
47: Queued,
48: Expired,
49: Executed
50: }
51:
53: mapping(uint => Proposal) public proposals; // <= FOUND
54:
56: mapping(address => uint) public latestProposalIds; // <= FOUND
57:
59: bytes32 public constant DOMAIN_TYPEHASH =
60: keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)");
61:
63: bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,bool support)");
64:
117: }
['53']
4: contract GovernorAlpha2 {
5:
6: string public constant name = "Venus Governor Alpha";
7:
24: TimelockInterface public timelock;
25:
27: XVSInterface public xvs;
28:
30: address public guardian;
31:
33: uint public proposalCount;
34:
41: enum ProposalState {
42: Pending,
43: Active,
44: Canceled,
45: Defeated,
46: Succeeded,
47: Queued,
48: Expired,
49: Executed
50: }
51:
53: mapping(uint => Proposal) public proposals; // <= FOUND
54:
56: mapping(address => uint) public latestProposalIds; // <= FOUND
57:
59: bytes32 public constant DOMAIN_TYPEHASH =
60: keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)");
61:
63: bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,bool support)");
64:
117: }
['111']
88: contract GovernorBravoDelegateStorageV1 is GovernorBravoDelegatorStorage {
89:
90: uint public votingDelay;
91:
93: uint public votingPeriod;
94:
96: uint public proposalThreshold;
97:
99: uint public initialProposalId;
100:
102: uint public proposalCount;
103:
105: TimelockInterface public timelock;
106:
108: XvsVaultInterface public xvsVault;
109:
111: mapping(uint => Proposal) public proposals; // <= FOUND
112:
114: mapping(address => uint) public latestProposalIds; // <= FOUND
115:
122: enum ProposalState {
123: Pending,
124: Active,
125: Canceled,
126: Defeated,
127: Succeeded,
128: Queued,
129: Expired,
130: Executed
131: }
132:
134: uint public proposalMaxOperations;
135:
137: address public guardian;
138: }
['111']
88: contract GovernorBravoDelegateStorageV1 is GovernorBravoDelegatorStorage {
89:
90: uint public votingDelay;
91:
93: uint public votingPeriod;
94:
96: uint public proposalThreshold;
97:
99: uint public initialProposalId;
100:
102: uint public proposalCount;
103:
105: TimelockInterface public timelock;
106:
108: XvsVaultInterface public xvsVault;
109:
111: mapping(uint => Proposal) public proposals; // <= FOUND
112:
114: mapping(address => uint) public latestProposalIds; // <= FOUND
115:
122: enum ProposalState {
123: Pending,
124: Active,
125: Canceled,
126: Defeated,
127: Succeeded,
128: Queued,
129: Expired,
130: Executed
131: }
132:
134: uint public proposalMaxOperations;
135:
137: address public guardian;
138: }
['196']
186: contract GovernorBravoDelegateStorageV2 is GovernorBravoDelegateStorageV1 {
187: enum ProposalType {
188: NORMAL,
189: FASTTRACK,
190: CRITICAL
191: }
192:
196: mapping(uint => ProposalConfig) public proposalConfigs; // <= FOUND
197:
199: mapping(uint => TimelockInterface) public proposalTimelocks; // <= FOUND
200: }
['56']
19: contract OmnichainGovernanceExecutor is ReentrancyGuard, BaseOmnichainControllerDest {
20: using BytesLib for bytes;
21: using ExcessivelySafeCall for address;
22:
23: enum ProposalType {
24: NORMAL,
25: FASTTRACK,
26: CRITICAL
27: }
28:
32: enum ProposalState {
33: Canceled,
34: Queued,
35: Executed
36: }
37:
41: address public immutable GUARDIAN;
42:
46: uint16 public srcChainId;
47:
51: uint256 public lastProposalReceived;
52:
56: mapping(uint256 => Proposal) public proposals; // <= FOUND
57:
61: mapping(uint256 => ITimelock) public proposalTimelocks; // <= FOUND
62:
66: mapping(uint256 => bool) public queued; // <= FOUND
67:
182: }
['29']
19: contract OmnichainProposalSender is ReentrancyGuard, BaseOmnichainControllerSrc {
20:
23: uint256 public proposalCount;
24:
29: mapping(uint256 => bytes32) public storedExecutionHashes; // <= FOUND
30:
34: ILayerZeroEndpoint public immutable LZ_ENDPOINT;
35:
39: mapping(uint16 => bytes) public trustedRemoteLookup; // <= FOUND
40:
165: }
Putting constants on the left side of a comparison operator like == or < is a best practice known as "Yoda conditions", which can help prevent accidental assignment instead of comparison. In some programming languages, if a variable is mistakenly put on the left with a single = instead of ==, it assigns the constant's value to the variable without any compiler error. However, doing this with the constant on the left would generate an error, as constants cannot be assigned values. Although Solidity's static typing system prevents accidental assignments within conditionals, adopting this practice enhances code readability and consistency, especially when developers are working across multiple languages that support this convention.
Num of instances: 5
Click to show findings
['164']
164: if (latestProposalId != 0) // <= FOUND
['164']
164: if (latestProposalId != 0) // <= FOUND
['363']
363: if (support == 0) // <= FOUND
['88']
88: if (a == 0) // <= FOUND
['235']
235: if (bytes(signature).length == 0) // <= FOUND
Modify such instances to include a capital I as the first character in the name to signify it is an interface. This improved readability during in
Num of instances: 5
Click to show findings
['391']
391: interface TimelockInterface
['425']
425: interface XVSInterface
['391']
391: interface TimelockInterface
['248']
248: interface XvsVaultInterface
['252']
252: interface GovernorAlphaInterface
Emitting an event within initializer functions in Solidity is a best practice for providing transparency and traceability. Initializer functions set the initial state and values of an upgradeable contract. Emitting an event during initialization allows anyone to verify and audit the initial state of the contract via the transaction logs. This can be particularly useful for verifying the parameters set during initialization, tracking the contract's deployment, and troubleshooting or debugging. Therefore, developers should include an event emission in their initializer functions, providing a clear record of the contract's initialization and enhancing the contract's transparency and security.
Num of instances: 3
Click to show findings
['111']
111: function initialize( // <= FOUND
112: address xvsVault_,
113: ProposalConfig[] memory proposalConfigs_,
114: TimelockInterface[] memory timelocks,
115: address guardian_
116: ) public {
117: require(address(proposalTimelocks[0]) == address(0), "GovernorBravo::initialize: cannot initialize twice");
118: require(msg.sender == admin, "GovernorBravo::initialize: admin only");
119: require(xvsVault_ != address(0), "GovernorBravo::initialize: invalid xvs address");
120: require(guardian_ != address(0), "GovernorBravo::initialize: invalid guardian");
121: require(
122: timelocks.length == uint8(ProposalType.CRITICAL) + 1,
123: "GovernorBravo::initialize:number of timelocks should match number of governance routes"
124: );
125: require(
126: proposalConfigs_.length == uint8(ProposalType.CRITICAL) + 1,
127: "GovernorBravo::initialize:number of proposal configs should match number of governance routes"
128: );
129:
130: xvsVault = XvsVaultInterface(xvsVault_);
131: proposalMaxOperations = 10;
132: guardian = guardian_;
133:
134:
135: uint256 arrLength = proposalConfigs_.length;
136: for (uint256 i; i < arrLength; ++i) {
137: require(
138: proposalConfigs_[i].votingPeriod >= MIN_VOTING_PERIOD,
139: "GovernorBravo::initialize: invalid min voting period"
140: );
141: require(
142: proposalConfigs_[i].votingPeriod <= MAX_VOTING_PERIOD,
143: "GovernorBravo::initialize: invalid max voting period"
144: );
145: require(
146: proposalConfigs_[i].votingDelay >= MIN_VOTING_DELAY,
147: "GovernorBravo::initialize: invalid min voting delay"
148: );
149: require(
150: proposalConfigs_[i].votingDelay <= MAX_VOTING_DELAY,
151: "GovernorBravo::initialize: invalid max voting delay"
152: );
153: require(
154: proposalConfigs_[i].proposalThreshold >= MIN_PROPOSAL_THRESHOLD,
155: "GovernorBravo::initialize: invalid min proposal threshold"
156: );
157: require(
158: proposalConfigs_[i].proposalThreshold <= MAX_PROPOSAL_THRESHOLD,
159: "GovernorBravo::initialize: invalid max proposal threshold"
160: );
['51']
51: function initialize( // <= FOUND
52: address timelock_,
53: address xvsVault_,
54: uint votingPeriod_,
55: uint votingDelay_,
56: uint proposalThreshold_,
57: address guardian_
58: ) public {
59: require(address(timelock) == address(0), "GovernorBravo::initialize: can only initialize once");
60: require(msg.sender == admin, "GovernorBravo::initialize: admin only");
61: require(timelock_ != address(0), "GovernorBravo::initialize: invalid timelock address");
62: require(xvsVault_ != address(0), "GovernorBravo::initialize: invalid xvs address");
63: require(
64: votingPeriod_ >= MIN_VOTING_PERIOD && votingPeriod_ <= MAX_VOTING_PERIOD,
65: "GovernorBravo::initialize: invalid voting period"
66: );
67: require(
68: votingDelay_ >= MIN_VOTING_DELAY && votingDelay_ <= MAX_VOTING_DELAY,
69: "GovernorBravo::initialize: invalid voting delay"
70: );
71: require(
72: proposalThreshold_ >= MIN_PROPOSAL_THRESHOLD && proposalThreshold_ <= MAX_PROPOSAL_THRESHOLD,
73: "GovernorBravo::initialize: invalid proposal threshold"
74: );
75: require(guardian_ != address(0), "GovernorBravo::initialize: invalid guardian");
76:
77: timelock = TimelockInterface(timelock_);
78: xvsVault = XvsVaultInterface(xvsVault_);
79: votingPeriod = votingPeriod_;
80: votingDelay = votingDelay_;
81: proposalThreshold = proposalThreshold_;
82: proposalMaxOperations = 10;
83: guardian = guardian_;
84: }
['43']
43: function initialize(address accessControlManager_) external initializer { // <= FOUND
44: require(accessControlManager_ != address(0), "Address must not be zero");
45: __AccessControlled_init(accessControlManager_);
46: }
Make found instants CAPITAL_CASE
Num of instances: 3
Click to show findings
['6']
6: string public constant name = "Venus Governor Alpha"; // <= FOUND
['13']
13: string public constant name = "Venus Governor Bravo"; // <= FOUND
['34']
34: uint public constant quorumVotes = 600000e18; // <= FOUND
In Solidity version 0.8.18 and beyond mapping parameters can be named. This makes the purpose and function of a given mapping far clearer which in turn improves readability.
Num of instances: 10
Click to show findings
['85']
85: mapping(uint256 => bool) public queued; // <= FOUND
['71']
71: mapping(bytes32 => bool) public queuedTransactions; // <= FOUND
['25']
25: mapping(bytes4 => string) public functionRegistry; // <= FOUND
['39']
39: mapping(uint16 => bytes) public trustedRemoteLookup; // <= FOUND
['102']
102: mapping(address => uint) public latestProposalIds; // <= FOUND
['29']
29: mapping(uint256 => bytes32) public storedExecutionHashes; // <= FOUND
['26']
26: mapping(uint16 => uint256) public chainIdToMaxDailyLimit; // <= FOUND
['31']
31: mapping(uint16 => uint256) public chainIdToLast24HourCommandsSent; // <= FOUND
['36']
36: mapping(uint16 => uint256) public chainIdToLast24HourWindowStart; // <= FOUND
['40']
40: mapping(uint16 => uint256) public chainIdToLastProposalSentTimestamp; // <= FOUND
Num of instances: 6
Click to show findings
['6']
4: contract GovernorAlpha {
5:
6: string public constant name = "Venus Governor Alpha"; // <= FOUND
59: bytes32 public constant DOMAIN_TYPEHASH = // <= FOUND
63: bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,bool support)"); // <= FOUND
['6']
4: contract GovernorAlpha2 {
5:
6: string public constant name = "Venus Governor Alpha"; // <= FOUND
59: bytes32 public constant DOMAIN_TYPEHASH = // <= FOUND
63: bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,bool support)"); // <= FOUND
['75']
73: contract GovernorBravoDelegate is GovernorBravoDelegateStorageV2, GovernorBravoEvents {
74:
75: string public constant name = "Venus Governor Bravo"; // <= FOUND
78: uint public constant MIN_PROPOSAL_THRESHOLD = 150000e18; // <= FOUND
81: uint public constant MAX_PROPOSAL_THRESHOLD = 300000e18; // <= FOUND
84: uint public constant MIN_VOTING_PERIOD = 20 * 60 * 3; // <= FOUND
87: uint public constant MAX_VOTING_PERIOD = 20 * 60 * 24 * 14; // <= FOUND
90: uint public constant MIN_VOTING_DELAY = 1; // <= FOUND
93: uint public constant MAX_VOTING_DELAY = 20 * 60 * 24 * 7; // <= FOUND
96: uint public constant quorumVotes = 600000e18; // <= FOUND
99: bytes32 public constant DOMAIN_TYPEHASH = // <= FOUND
103: bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,uint8 support)"); // <= FOUND
['13']
11: contract GovernorBravoDelegateV1 is GovernorBravoDelegateStorageV1, GovernorBravoEventsV1 {
12:
13: string public constant name = "Venus Governor Bravo"; // <= FOUND
16: uint public constant MIN_PROPOSAL_THRESHOLD = 150000e18; // <= FOUND
19: uint public constant MAX_PROPOSAL_THRESHOLD = 300000e18; // <= FOUND
22: uint public constant MIN_VOTING_PERIOD = 20 * 60 * 3; // <= FOUND
25: uint public constant MAX_VOTING_PERIOD = 20 * 60 * 24 * 14; // <= FOUND
28: uint public constant MIN_VOTING_DELAY = 1; // <= FOUND
31: uint public constant MAX_VOTING_DELAY = 20 * 60 * 24 * 7; // <= FOUND
34: uint public constant quorumVotes = 600000e18; // <= FOUND
37: bytes32 public constant DOMAIN_TYPEHASH = // <= FOUND
41: bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,uint8 support)"); // <= FOUND
['31']
10: contract Timelock {
11: using SafeMath for uint;
12:
31: uint public constant GRACE_PERIOD = 14 days; // <= FOUND
34: uint public constant MINIMUM_DELAY = 1 hours; // <= FOUND
37: uint public constant MAXIMUM_DELAY = 30 days; // <= FOUND
['32']
12: contract TimelockV8 {
13:
32: uint256 private constant DEFAULT_GRACE_PERIOD = 14 days; // <= FOUND
35: uint256 private constant DEFAULT_MINIMUM_DELAY = 1 hours; // <= FOUND
38: uint256 private constant DEFAULT_MAXIMUM_DELAY = 30 days; // <= FOUND
Magic numbers should be avoided in Solidity code to enhance readability, maintainability, and reduce the likelihood of errors. Magic numbers are hard-coded values with no clear meaning or context, which can create confusion and make the code harder to understand for developers. Using well-defined constants or variables with descriptive names instead of magic numbers not only clarifies the purpose and significance of the value but also simplifies code updates and modifications.
Num of instances: 7
Click to show findings
['20']
20: return 10; // <= FOUND
['278']
278: (bool success, bytes memory reason) = address(this).excessivelySafeCall(
279: gasleft() - gasToStoreAndEmit,
280: 150, // <= FOUND
281: abi.encodeCall(this.nonblockingLzReceive, (srcChainId_, srcAddress_, nonce_, payload_))
282: );
['30']
30: return (60 * 60 * 24 * 3) / 3; // <= FOUND
['367']
367: } else if (support == 2) { // <= FOUND
['15']
15: return 300000e18; // <= FOUND
['20']
20: return 30; // <= FOUND
['10']
10: return 600000e18; // <= FOUND
Num of instances: 6
Click to show findings
['109']
109: function isAllowedToCall(address account, string calldata functionSig) public view returns (bool) { // <= FOUND
110: bytes32 role = keccak256(abi.encodePacked(msg.sender, functionSig));
111:
112: if (hasRole(role, account)) {
113: return true;
114: } else {
115: role = keccak256(abi.encodePacked(address(0), functionSig));
116: return hasRole(role, account);
117: }
118: }
['294']
294: function state(uint proposalId) public view returns (ProposalState) { // <= FOUND
295: require(proposalCount >= proposalId && proposalId > 0, "GovernorAlpha::state: invalid proposal id");
296: Proposal storage proposal = proposals[proposalId];
297: if (proposal.canceled) {
298: return ProposalState.Canceled;
299: } else if (block.number <= proposal.startBlock) {
300: return ProposalState.Pending;
301: } else if (block.number <= proposal.endBlock) {
302: return ProposalState.Active;
303: } else if (proposal.forVotes <= proposal.againstVotes || proposal.forVotes < quorumVotes()) {
304: return ProposalState.Defeated;
305: } else if (proposal.eta == 0) {
306: return ProposalState.Succeeded;
307: } else if (proposal.executed) {
308: return ProposalState.Executed;
309: } else if (block.timestamp >= add256(proposal.eta, timelock.GRACE_PERIOD())) {
310: return ProposalState.Expired;
311: } else {
312: return ProposalState.Queued;
313: }
314: }
['294']
294: function state(uint proposalId) public view returns (ProposalState) { // <= FOUND
295: require(proposalCount >= proposalId && proposalId > 0, "GovernorAlpha::state: invalid proposal id");
296: Proposal storage proposal = proposals[proposalId];
297: if (proposal.canceled) {
298: return ProposalState.Canceled;
299: } else if (block.number <= proposal.startBlock) {
300: return ProposalState.Pending;
301: } else if (block.number <= proposal.endBlock) {
302: return ProposalState.Active;
303: } else if (proposal.forVotes <= proposal.againstVotes || proposal.forVotes < quorumVotes()) {
304: return ProposalState.Defeated;
305: } else if (proposal.eta == 0) {
306: return ProposalState.Succeeded;
307: } else if (proposal.executed) {
308: return ProposalState.Executed;
309: } else if (block.timestamp >= add256(proposal.eta, timelock.GRACE_PERIOD())) {
310: return ProposalState.Expired;
311: } else {
312: return ProposalState.Queued;
313: }
314: }
['384']
384: function state(uint proposalId) public view returns (ProposalState) { // <= FOUND
385: require(
386: proposalCount >= proposalId && proposalId > initialProposalId,
387: "GovernorBravo::state: invalid proposal id"
388: );
389: Proposal storage proposal = proposals[proposalId];
390: if (proposal.canceled) {
391: return ProposalState.Canceled;
392: } else if (block.number <= proposal.startBlock) {
393: return ProposalState.Pending;
394: } else if (block.number <= proposal.endBlock) {
395: return ProposalState.Active;
396: } else if (proposal.forVotes <= proposal.againstVotes || proposal.forVotes < quorumVotes) {
397: return ProposalState.Defeated;
398: } else if (proposal.eta == 0) {
399: return ProposalState.Succeeded;
400: } else if (proposal.executed) {
401: return ProposalState.Executed;
402: } else if (
403: block.timestamp >= add256(proposal.eta, proposalTimelocks[uint8(proposal.proposalType)].GRACE_PERIOD())
404: ) {
405: return ProposalState.Expired;
406: } else {
407: return ProposalState.Queued;
408: }
409: }
['289']
289: function state(uint proposalId) public view returns (ProposalState) { // <= FOUND
290: require(
291: proposalCount >= proposalId && proposalId > initialProposalId,
292: "GovernorBravo::state: invalid proposal id"
293: );
294: Proposal storage proposal = proposals[proposalId];
295: if (proposal.canceled) {
296: return ProposalState.Canceled;
297: } else if (block.number <= proposal.startBlock) {
298: return ProposalState.Pending;
299: } else if (block.number <= proposal.endBlock) {
300: return ProposalState.Active;
301: } else if (proposal.forVotes <= proposal.againstVotes || proposal.forVotes < quorumVotes) {
302: return ProposalState.Defeated;
303: } else if (proposal.eta == 0) {
304: return ProposalState.Succeeded;
305: } else if (proposal.executed) {
306: return ProposalState.Executed;
307: } else if (block.timestamp >= add256(proposal.eta, timelock.GRACE_PERIOD())) {
308: return ProposalState.Expired;
309: } else {
310: return ProposalState.Queued;
311: }
312: }
['246']
246: function state(uint256 proposalId_) public view returns (ProposalState) { // <= FOUND
247: Proposal storage proposal = proposals[proposalId_];
248: if (proposal.canceled) {
249: return ProposalState.Canceled;
250: } else if (proposal.executed) {
251: return ProposalState.Executed;
252: } else if (queued[proposalId_]) {
253:
254: return ProposalState.Queued;
255: } else {
256: revert InvalidProposalId();
257: }
258: }
[NonCritical-44] Inconsistent usage of int/uint with int256/uint256 in contract/abstract/library/interface
Use uint256/int256 consistently in place of uint/int to prevent ambiguity rather than using both within the same contract/abstract/library/interface
Num of instances: 7
Click to show findings
['11']
11: contract GovernorBravoDelegatorV1 is GovernorBravoDelegatorStorage, GovernorBravoEventsV1 {
12: constructor(
17: uint votingPeriod_, // <= FOUND
18: uint votingDelay_, // <= FOUND
19: uint proposalThreshold_, // <= FOUND
28: "initialize(address,address,uint256,uint256,uint256,address)", // <= FOUND
['4']
4: contract GovernorAlpha {
5:
43: uint public proposalCount; // <= FOUND
47: uint id; // <= FOUND
51: uint eta; // <= FOUND
61: uint startBlock; // <= FOUND
63: uint endBlock; // <= FOUND
65: uint forVotes; // <= FOUND
67: uint againstVotes; // <= FOUND
99: mapping(uint => Proposal) public proposals; // <= FOUND
106: keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)"); // <= FOUND
109: bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,bool support)"); // <= FOUND
113: uint id, // <= FOUND
119: uint startBlock, // <= FOUND
120: uint endBlock, // <= FOUND
125: event VoteCast(address voter, uint proposalId, bool support, uint votes); // <= FOUND
128: event ProposalCanceled(uint id); // <= FOUND
131: event ProposalQueued(uint id, uint eta); // <= FOUND
134: event ProposalExecuted(uint id); // <= FOUND
162: uint latestProposalId = latestProposalIds[msg.sender]; // <= FOUND
175: uint startBlock = add256(block.number, votingDelay()); // <= FOUND
176: uint endBlock = add256(startBlock, votingPeriod()); // <= FOUND
212: function queue(uint proposalId) public { // <= FOUND
218: uint eta = add256(block.timestamp, timelock.delay()); // <= FOUND
219: for (uint i = 0; i < proposal.targets.length; i++) { // <= FOUND
226: function _queueOrRevert(address target, uint value, string memory signature, bytes memory data, uint eta) internal { // <= FOUND
234: function execute(uint proposalId) public payable { // <= FOUND
241: for (uint i = 0; i < proposal.targets.length; i++) { // <= FOUND
253: function cancel(uint proposalId) public { // <= FOUND
265: for (uint i = 0; i < proposal.targets.length; i++) { // <= FOUND
279: uint proposalId // <= FOUND
289: function getReceipt(uint proposalId, address voter) public view returns (Receipt memory) { // <= FOUND
293: function state(uint proposalId) public view returns (ProposalState) { // <= FOUND
315: function castVote(uint proposalId, bool support) public { // <= FOUND
319: function castVoteBySig(uint proposalId, bool support, uint8 v, bytes32 r, bytes32 s) public { // <= FOUND
330: function _castVote(address voter, uint proposalId, bool support) internal { // <= FOUND
360: function __queueSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public { // <= FOUND
365: function __executeSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public { // <= FOUND
370: function add256(uint256 a, uint256 b) internal pure returns (uint) { // <= FOUND
371: uint c = a + b; // <= FOUND
376: function sub256(uint256 a, uint256 b) internal pure returns (uint) { // <= FOUND
382: uint chainId; // <= FOUND
['4']
4: contract GovernorAlpha2 {
5:
43: uint public proposalCount; // <= FOUND
47: uint id; // <= FOUND
51: uint eta; // <= FOUND
61: uint startBlock; // <= FOUND
63: uint endBlock; // <= FOUND
65: uint forVotes; // <= FOUND
67: uint againstVotes; // <= FOUND
99: mapping(uint => Proposal) public proposals; // <= FOUND
106: keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)"); // <= FOUND
109: bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,bool support)"); // <= FOUND
113: uint id, // <= FOUND
119: uint startBlock, // <= FOUND
120: uint endBlock, // <= FOUND
125: event VoteCast(address voter, uint proposalId, bool support, uint votes); // <= FOUND
128: event ProposalCanceled(uint id); // <= FOUND
131: event ProposalQueued(uint id, uint eta); // <= FOUND
134: event ProposalExecuted(uint id); // <= FOUND
136: constructor(address timelock_, address xvs_, address guardian_, uint256 lastProposalId_) public { // <= FOUND
163: uint latestProposalId = latestProposalIds[msg.sender]; // <= FOUND
176: uint startBlock = add256(block.number, votingDelay()); // <= FOUND
177: uint endBlock = add256(startBlock, votingPeriod()); // <= FOUND
213: function queue(uint proposalId) public { // <= FOUND
219: uint eta = add256(block.timestamp, timelock.delay()); // <= FOUND
220: for (uint i = 0; i < proposal.targets.length; i++) { // <= FOUND
227: function _queueOrRevert(address target, uint value, string memory signature, bytes memory data, uint eta) internal { // <= FOUND
235: function execute(uint proposalId) public payable { // <= FOUND
242: for (uint i = 0; i < proposal.targets.length; i++) { // <= FOUND
254: function cancel(uint proposalId) public { // <= FOUND
266: for (uint i = 0; i < proposal.targets.length; i++) { // <= FOUND
280: uint proposalId // <= FOUND
290: function getReceipt(uint proposalId, address voter) public view returns (Receipt memory) { // <= FOUND
294: function state(uint proposalId) public view returns (ProposalState) { // <= FOUND
316: function castVote(uint proposalId, bool support) public { // <= FOUND
320: function castVoteBySig(uint proposalId, bool support, uint8 v, bytes32 r, bytes32 s) public { // <= FOUND
331: function _castVote(address voter, uint proposalId, bool support) internal { // <= FOUND
361: function __queueSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public { // <= FOUND
366: function __executeSetTimelockPendingAdmin(address newPendingAdmin, uint eta) public { // <= FOUND
371: function add256(uint256 a, uint256 b) internal pure returns (uint) { // <= FOUND
372: uint c = a + b; // <= FOUND
377: function sub256(uint256 a, uint256 b) internal pure returns (uint) { // <= FOUND
383: uint chainId; // <= FOUND
['73']
73: contract GovernorBravoDelegate is GovernorBravoDelegateStorageV2, GovernorBravoEvents {
74:
78: uint public constant MIN_PROPOSAL_THRESHOLD = 150000e18; // <= FOUND
81: uint public constant MAX_PROPOSAL_THRESHOLD = 300000e18; // <= FOUND
84: uint public constant MIN_VOTING_PERIOD = 20 * 60 * 3; // <= FOUND
87: uint public constant MAX_VOTING_PERIOD = 20 * 60 * 24 * 14; // <= FOUND
90: uint public constant MIN_VOTING_DELAY = 1; // <= FOUND
93: uint public constant MAX_VOTING_DELAY = 20 * 60 * 24 * 7; // <= FOUND
96: uint public constant quorumVotes = 600000e18; // <= FOUND
100: keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)"); // <= FOUND
103: bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,uint8 support)"); // <= FOUND
135: uint256 arrLength = proposalConfigs_.length; // <= FOUND
136: for (uint256 i; i < arrLength; ++i) { // <= FOUND
205: uint latestProposalId = latestProposalIds[msg.sender]; // <= FOUND
218: uint startBlock = add256(block.number, proposalConfigs[uint8(proposalType)].votingDelay); // <= FOUND
219: uint endBlock = add256(startBlock, proposalConfigs[uint8(proposalType)].votingPeriod); // <= FOUND
262: function queue(uint proposalId) external { // <= FOUND
268: uint eta = add256(block.timestamp, proposalTimelocks[uint8(proposal.proposalType)].delay()); // <= FOUND
269: for (uint i; i < proposal.targets.length; ++i) { // <= FOUND
285: uint value, // <= FOUND
288: uint eta, // <= FOUND
304: function execute(uint proposalId) external { // <= FOUND
311: for (uint i; i < proposal.targets.length; ++i) { // <= FOUND
327: function cancel(uint proposalId) external { // <= FOUND
340: for (uint i = 0; i < proposal.targets.length; i++) { // <= FOUND
359: uint proposalId // <= FOUND
375: function getReceipt(uint proposalId, address voter) external view returns (Receipt memory) { // <= FOUND
384: function state(uint proposalId) public view returns (ProposalState) { // <= FOUND
416: function castVote(uint proposalId, uint8 support) external { // <= FOUND
426: function castVoteWithReason(uint proposalId, uint8 support, string calldata reason) external { // <= FOUND
439: function castVoteBySig(uint proposalId, uint8 support, uint8 v, bytes32 r, bytes32 s) external { // <= FOUND
457: function castVoteInternal(address voter, uint proposalId, uint8 support) internal returns (uint96) { // <= FOUND
503: for (uint256 i; i < uint8(ProposalType.CRITICAL) + 1; ++i) { // <= FOUND
513: function _setProposalMaxOperations(uint proposalMaxOperations_) external { // <= FOUND
515: uint oldProposalMaxOperations = proposalMaxOperations; // <= FOUND
565: function add256(uint256 a, uint256 b) internal pure returns (uint) { // <= FOUND
566: uint c = a + b; // <= FOUND
571: function sub256(uint256 a, uint256 b) internal pure returns (uint) { // <= FOUND
577: uint chainId; // <= FOUND
['11']
11: contract GovernorBravoDelegateV1 is GovernorBravoDelegateStorageV1, GovernorBravoEventsV1 {
12:
16: uint public constant MIN_PROPOSAL_THRESHOLD = 150000e18; // <= FOUND
19: uint public constant MAX_PROPOSAL_THRESHOLD = 300000e18; // <= FOUND
22: uint public constant MIN_VOTING_PERIOD = 20 * 60 * 3; // <= FOUND
25: uint public constant MAX_VOTING_PERIOD = 20 * 60 * 24 * 14; // <= FOUND
28: uint public constant MIN_VOTING_DELAY = 1; // <= FOUND
31: uint public constant MAX_VOTING_DELAY = 20 * 60 * 24 * 7; // <= FOUND
34: uint public constant quorumVotes = 600000e18; // <= FOUND
38: keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)"); // <= FOUND
41: bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,uint8 support)"); // <= FOUND
54: uint votingPeriod_, // <= FOUND
55: uint votingDelay_, // <= FOUND
56: uint proposalThreshold_, // <= FOUND
117: uint latestProposalId = latestProposalIds[msg.sender]; // <= FOUND
130: uint startBlock = add256(block.number, votingDelay); // <= FOUND
131: uint endBlock = add256(startBlock, votingPeriod); // <= FOUND
172: function queue(uint proposalId) external { // <= FOUND
178: uint eta = add256(block.timestamp, timelock.delay()); // <= FOUND
179: for (uint i = 0; i < proposal.targets.length; i++) { // <= FOUND
194: uint value, // <= FOUND
197: uint eta // <= FOUND
210: function execute(uint proposalId) external { // <= FOUND
217: for (uint i = 0; i < proposal.targets.length; i++) { // <= FOUND
233: function cancel(uint proposalId) external { // <= FOUND
245: for (uint i = 0; i < proposal.targets.length; i++) { // <= FOUND
264: uint proposalId // <= FOUND
280: function getReceipt(uint proposalId, address voter) external view returns (Receipt memory) { // <= FOUND
289: function state(uint proposalId) public view returns (ProposalState) { // <= FOUND
319: function castVote(uint proposalId, uint8 support) external { // <= FOUND
329: function castVoteWithReason(uint proposalId, uint8 support, string calldata reason) external { // <= FOUND
337: function castVoteBySig(uint proposalId, uint8 support, uint8 v, bytes32 r, bytes32 s) external { // <= FOUND
355: function castVoteInternal(address voter, uint proposalId, uint8 support) internal returns (uint96) { // <= FOUND
395: function _setVotingDelay(uint newVotingDelay) external { // <= FOUND
401: uint oldVotingDelay = votingDelay; // <= FOUND
411: function _setVotingPeriod(uint newVotingPeriod) external { // <= FOUND
417: uint oldVotingPeriod = votingPeriod; // <= FOUND
428: function _setProposalThreshold(uint newProposalThreshold) external { // <= FOUND
434: uint oldProposalThreshold = proposalThreshold; // <= FOUND
458: function _setProposalMaxOperations(uint proposalMaxOperations_) external { // <= FOUND
460: uint oldProposalMaxOperations = proposalMaxOperations; // <= FOUND
510: function add256(uint256 a, uint256 b) internal pure returns (uint) { // <= FOUND
511: uint c = a + b; // <= FOUND
516: function sub256(uint256 a, uint256 b) internal pure returns (uint) { // <= FOUND
522: uint chainId; // <= FOUND
['11']
11: contract GovernorBravoDelegator is GovernorBravoDelegatorStorage, GovernorBravoEvents {
12: constructor(
17: uint votingPeriod_, // <= FOUND
18: uint votingDelay_, // <= FOUND
19: uint proposalThreshold_, // <= FOUND
28: "initialize(address,address,uint256,uint256,uint256,address)", // <= FOUND
['186']
186: contract GovernorBravoDelegateStorageV2 is GovernorBravoDelegateStorageV1 {
187: enum ProposalType {
195: uint256 votingDelay; // <= FOUND
197: uint256 votingPeriod; // <= FOUND
199: uint256 proposalThreshold; // <= FOUND
203: mapping(uint => ProposalConfig) public proposalConfigs; // <= FOUND
206: mapping(uint => TimelockInterface) public proposalTimelocks; // <= FOUND
It's crucial to leverage the right features for the appropriate contexts in Solidity, despite the compiler's ability to correct common developer mistakes. Both constant and immutable variables have distinct uses. Constant variables are best suited for hard-coded, literal values within your contract, where the value doesn't need to be computed or modified.
On the other hand, immutable variables are ideal for situations where the value might be the result of an expression or received from a constructor. While both serve to define unchanging variables, they function differently and their proper utilization contributes to clearer, more efficient code. Remember, even if the compiler can correct certain mistakes, best practices dictate using the correct feature for the task at hand.
Num of instances: 2
Click to show findings
['109']
109: bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,bool support)"); // <= FOUND
['41']
41: bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,uint8 support)"); // <= FOUND
In the event of a security breach or any unforeseen emergency, swiftly suspending all protocol operations becomes crucial. Having a mechanism in place to halt all functions collectively, instead of pausing individual contracts separately, substantially enhances the efficiency of mitigating ongoing attacks or vulnerabilities. This not only quickens the response time to potential threats but also reduces operational stress during these critical periods. Therefore, consider integrating a 'circuit breaker' or 'emergency stop' function into the smart contract system architecture. Such a feature would provide the capability to suspend the entire protocol instantly, which could prove invaluable during a time-sensitive crisis management situation.
Num of instances: 19
Click to show findings
['12']
12: contract AccessControlledV5
['52']
52: contract AccessControlManager is AccessControl, IAccessControlManagerV8
['11']
11: contract GovernorBravoDelegatorV1 is GovernorBravoDelegatorStorage, GovernorBravoEventsV1
['4']
4: contract GovernorAlpha
['4']
4: contract GovernorAlpha2
['73']
73: contract GovernorBravoDelegate is GovernorBravoDelegateStorageV2, GovernorBravoEvents
['11']
11: contract GovernorBravoDelegateV1 is GovernorBravoDelegateStorageV1, GovernorBravoEventsV1
['11']
11: contract GovernorBravoDelegator is GovernorBravoDelegatorStorage, GovernorBravoEvents
['9']
9: contract GovernorBravoEvents
['73']
73: contract GovernorBravoDelegatorStorage
['88']
88: contract GovernorBravoDelegateStorageV1 is GovernorBravoDelegatorStorage
['186']
186: contract GovernorBravoDelegateStorageV2 is GovernorBravoDelegateStorageV1
['12']
12: contract GovernorBravoEventsV1
['88']
88: contract GovernorBravoDelegateStorageV1 is GovernorBravoDelegatorStorage
['16']
16: contract OmnichainExecutorOwner is AccessControlledV8
['19']
19: contract OmnichainGovernanceExecutor is ReentrancyGuard, BaseOmnichainControllerDest
['19']
19: contract OmnichainProposalSender is ReentrancyGuard, BaseOmnichainControllerSrc
['15']
15: abstract contract AccessControlledV8 is Initializable, Ownable2StepUpgradeable
['16']
16: library SafeMath
Smart contracts are complex entities, and clarity in their operations is fundamental to ensure that they function as intended. Casting a single argument instead of utilizing 'abi.encodePacked()' improves the transparency of the operation. It elucidates the intent of the code, reducing ambiguity and making it easier for auditors and developers to understand the code’s purpose. Such practices promote readability and maintainability, thus reducing the likelihood of errors and misunderstandings. Therefore, it's recommended to employ explicit casts for single arguments where possible, to increase the contract's comprehensibility and ensure a smoother review process.
Num of instances: 8
Click to show findings
['78']
77: function giveCallPermission(address contractAddress, string calldata functionSig, address accountToPermit) public {
78: bytes32 role = keccak256(abi.encodePacked(contractAddress, functionSig)); // <= FOUND
79: grantRole(role, accountToPermit);
80: emit PermissionGranted(accountToPermit, contractAddress, functionSig);
81: }
['96']
91: function revokeCallPermission(
92: address contractAddress,
93: string calldata functionSig,
94: address accountToRevoke
95: ) public {
96: bytes32 role = keccak256(abi.encodePacked(contractAddress, functionSig)); // <= FOUND
97: revokeRole(role, accountToRevoke);
98: emit PermissionRevoked(accountToRevoke, contractAddress, functionSig);
99: }
['110']
109: function isAllowedToCall(address account, string calldata functionSig) public view returns (bool) {
110: bytes32 role = keccak256(abi.encodePacked(msg.sender, functionSig)); // <= FOUND
111:
112: if (hasRole(role, account)) {
113: return true;
114: } else {
115: role = keccak256(abi.encodePacked(address(0), functionSig)); // <= FOUND
116: return hasRole(role, account);
117: }
118: }
['133']
128: function hasPermission(
129: address account,
130: address contractAddress,
131: string calldata functionSig
132: ) public view returns (bool) {
133: bytes32 role = keccak256(abi.encodePacked(contractAddress, functionSig)); // <= FOUND
134: return hasRole(role, account);
135: }
['325']
320: function castVoteBySig(uint proposalId, bool support, uint8 v, bytes32 r, bytes32 s) public {
321: bytes32 domainSeparator = keccak256(
322: abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name)), getChainId(), address(this))
323: );
324: bytes32 structHash = keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support));
325: bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash)); // <= FOUND
326: address signatory = ecrecover(digest, v, r, s);
327: require(signatory != address(0), "GovernorAlpha::castVoteBySig: invalid signature");
328: return _castVote(signatory, proposalId, support);
329: }
['325']
320: function castVoteBySig(uint proposalId, bool support, uint8 v, bytes32 r, bytes32 s) public {
321: bytes32 domainSeparator = keccak256(
322: abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name)), getChainId(), address(this))
323: );
324: bytes32 structHash = keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support));
325: bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash)); // <= FOUND
326: address signatory = ecrecover(digest, v, r, s);
327: require(signatory != address(0), "GovernorAlpha::castVoteBySig: invalid signature");
328: return _castVote(signatory, proposalId, support);
329: }
['342']
337: function castVoteBySig(uint proposalId, uint8 support, uint8 v, bytes32 r, bytes32 s) external {
338: bytes32 domainSeparator = keccak256(
339: abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name)), getChainIdInternal(), address(this))
340: );
341: bytes32 structHash = keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support));
342: bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash)); // <= FOUND
343: address signatory = ecrecover(digest, v, r, s);
344: require(signatory != address(0), "GovernorBravo::castVoteBySig: invalid signature");
345: emit VoteCast(signatory, proposalId, support, castVoteInternal(signatory, proposalId, support), "");
346: }
['342']
337: function castVoteBySig(uint proposalId, uint8 support, uint8 v, bytes32 r, bytes32 s) external {
338: bytes32 domainSeparator = keccak256(
339: abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name)), getChainIdInternal(), address(this))
340: );
341: bytes32 structHash = keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support));
342: bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash)); // <= FOUND
343: address signatory = ecrecover(digest, v, r, s);
344: require(signatory != address(0), "GovernorBravo::castVoteBySig: invalid signature");
345: emit VoteCast(signatory, proposalId, support, castVoteInternal(signatory, proposalId, support), "");
346: }
The codebase features instances where low-level assembly is used to access properties like chain ID, external contract size, and external contract hash. However, since Solidity provides native high-level constructs to access these properties, it is recommended to use those for better readability, maintainability, and reduced complexity. For example, assembly{ id := chainid() } can be replaced with uint256 id = block.chainid, assembly { size := extcodesize() } can be substituted with uint256 size = address().code.length, and assembly { hash := extcodehash() } can be transformed into bytes32 hash = address().codehash. The usage of inline assembly can often flag the project as more complex by automated analysis tools and therefore, should be avoided where high-level constructs can achieve the same functionality.
Num of instances: 1
When developing smart contracts in Solidity, it's crucial to validate the inputs of your functions. This includes ensuring that the bytes parameters are not empty, especially when they represent crucial data such as addresses, identifiers, or raw data that the contract needs to process.
Missing empty bytes checks can lead to unexpected behaviour in your contract. For instance, certain operations might fail, produce incorrect results, or consume unnecessary gas when performed with empty bytes. Moreover, missing input validation can potentially expose your contract to malicious activity, including exploitation of unhandled edge cases.
To mitigate these issues, always validate that bytes parameters are not empty when the logic of your contract requires it.
Num of instances: 19
Click to show findings
['66']
66: function delegateTo(address callee, bytes memory data) internal {
67: (bool success, bytes memory returnData) = callee.delegatecall(data);
68: assembly {
69: if eq(success, 0) {
70: revert(add(returnData, 0x20), returndatasize)
71: }
72: }
73: }
['227']
227: function _queueOrRevert(address target, uint value, string memory signature, bytes memory data, uint eta) internal {
228: require(
229: !timelock.queuedTransactions(keccak256(abi.encode(target, value, signature, data, eta))),
230: "GovernorAlpha::_queueOrRevert: proposal action already queued at eta"
231: );
232: timelock.queueTransaction(target, value, signature, data, eta);
233: }
['320']
320: function castVoteBySig(uint proposalId, bool support, uint8 v, bytes32 r, bytes32 s) public {
321: bytes32 domainSeparator = keccak256(
322: abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name)), getChainId(), address(this))
323: );
324: bytes32 structHash = keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support));
325: bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
326: address signatory = ecrecover(digest, v, r, s);
327: require(signatory != address(0), "GovernorAlpha::castVoteBySig: invalid signature");
328: return _castVote(signatory, proposalId, support);
329: }
['227']
227: function _queueOrRevert(address target, uint value, string memory signature, bytes memory data, uint eta) internal {
228: require(
229: !timelock.queuedTransactions(keccak256(abi.encode(target, value, signature, data, eta))),
230: "GovernorAlpha::_queueOrRevert: proposal action already queued at eta"
231: );
232: timelock.queueTransaction(target, value, signature, data, eta);
233: }
['320']
320: function castVoteBySig(uint proposalId, bool support, uint8 v, bytes32 r, bytes32 s) public {
321: bytes32 domainSeparator = keccak256(
322: abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name)), getChainId(), address(this))
323: );
324: bytes32 structHash = keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support));
325: bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
326: address signatory = ecrecover(digest, v, r, s);
327: require(signatory != address(0), "GovernorAlpha::castVoteBySig: invalid signature");
328: return _castVote(signatory, proposalId, support);
329: }
['283']
283: function queueOrRevertInternal(
284: address target,
285: uint value,
286: string memory signature,
287: bytes memory data,
288: uint eta,
289: uint8 proposalType
290: ) internal {
291: require(
292: !proposalTimelocks[proposalType].queuedTransactions(
293: keccak256(abi.encode(target, value, signature, data, eta))
294: ),
295: "GovernorBravo::queueOrRevertInternal: identical proposal action already queued at eta"
296: );
297: proposalTimelocks[proposalType].queueTransaction(target, value, signature, data, eta);
298: }
['337']
337: function castVoteBySig(uint proposalId, uint8 support, uint8 v, bytes32 r, bytes32 s) external {
338: bytes32 domainSeparator = keccak256(
339: abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name)), getChainIdInternal(), address(this))
340: );
341: bytes32 structHash = keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support));
342: bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
343: address signatory = ecrecover(digest, v, r, s);
344: require(signatory != address(0), "GovernorBravo::castVoteBySig: invalid signature");
345: emit VoteCast(signatory, proposalId, support, castVoteInternal(signatory, proposalId, support), "");
346: }
['192']
192: function queueOrRevertInternal(
193: address target,
194: uint value,
195: string memory signature,
196: bytes memory data,
197: uint eta
198: ) internal {
199: require(
200: !timelock.queuedTransactions(keccak256(abi.encode(target, value, signature, data, eta))),
201: "GovernorBravo::queueOrRevertInternal: identical proposal action already queued at eta"
202: );
203: timelock.queueTransaction(target, value, signature, data, eta);
204: }
['337']
337: function castVoteBySig(uint proposalId, uint8 support, uint8 v, bytes32 r, bytes32 s) external {
338: bytes32 domainSeparator = keccak256(
339: abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name)), getChainIdInternal(), address(this))
340: );
341: bytes32 structHash = keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support));
342: bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));
343: address signatory = ecrecover(digest, v, r, s);
344: require(signatory != address(0), "GovernorBravo::castVoteBySig: invalid signature");
345: emit VoteCast(signatory, proposalId, support, castVoteInternal(signatory, proposalId, support), "");
346: }
['66']
66: function delegateTo(address callee, bytes memory data) internal {
67: (bool success, bytes memory returnData) = callee.delegatecall(data);
68: assembly {
69: if eq(success, 0) {
70: revert(add(returnData, 0x20), returndatasize)
71: }
72: }
73: }
['268']
268: function _blockingLzReceive(
269: uint16 srcChainId_,
270: bytes memory srcAddress_,
271: uint64 nonce_,
272: bytes memory payload_
273: ) internal virtual override whenNotPaused {
274: uint256 gasToStoreAndEmit = 30000;
275:
276: require(srcChainId_ == srcChainId, "OmnichainGovernanceExecutor::_blockingLzReceive: invalid source chain id");
277:
278: (bool success, bytes memory reason) = address(this).excessivelySafeCall(
279: gasleft() - gasToStoreAndEmit,
280: 150,
281: abi.encodeCall(this.nonblockingLzReceive, (srcChainId_, srcAddress_, nonce_, payload_))
282: );
283:
284: if (!success) {
285: bytes32 hashedPayload = keccak256(payload_);
286: failedMessages[srcChainId_][srcAddress_][nonce_] = hashedPayload;
287: emit ReceivePayloadFailed(srcChainId_, srcAddress_, nonce_, reason);
288: }
289: }
['376']
376: function _queueOrRevertInternal(
377: address target_,
378: uint256 value_,
379: string memory signature_,
380: bytes memory data_,
381: uint256 eta_,
382: uint8 proposalType_
383: ) internal {
384: require(
385: !proposalTimelocks[proposalType_].queuedTransactions(
386: keccak256(abi.encode(target_, value_, signature_, data_, eta_))
387: ),
388: "OmnichainGovernanceExecutor::queueOrRevertInternal: identical proposal action already queued at eta"
389: );
390:
391: proposalTimelocks[proposalType_].queueTransaction(target_, value_, signature_, data_, eta_);
392: }
['94']
94: function estimateFees(
95: uint16 remoteChainId_,
96: bytes calldata payload_,
97: bytes calldata adapterParams_
98: ) external view returns (uint256, uint256) {
99: return LZ_ENDPOINT.estimateFees(remoteChainId_, address(this), payload_, false, adapterParams_);
100: }
['249']
249: function setTrustedRemoteAddress(uint16 remoteChainId_, bytes calldata newRemoteAdd