This is the top-ranked automated findings report, from henry bot. All findings in this report will be considered known issues for the purposes of your C4 audit.
Audit report for 2023-10-ens generated by Bot-Henry.
Note: There is a section for disputed findings below the usual findings sections.
Total 1 instance over 1 issue:
| ID | Issue | Instances |
|---|---|---|
| [M‑01] | Centralization risk for privileged functions | 1 |
Total 22 instances over 10 issues:
| ID | Issue | Instances |
|---|---|---|
| [L‑01] | Use Ownable2Step instead of Ownable | 1 |
| [L‑02] | Missing zero address check in constructor | 3 |
| [L‑03] | Solidity version 0.8.20 or above may not work on other chains due to PUSH0 | 1 |
| [L‑04] | Vulnerable versions of packages are being used | 6 |
| [L‑05] | Missing checks for address(0) when setting address state variables |
1 |
| [L‑06] | Variables shadowing other definitions | 1 |
| [L‑07] | Owner can renounce Ownership | 1 |
| [L‑08] | Constructor / initialization function lacks parameter validation | 3 |
| [L‑09] | External function calls within loops | 2 |
| [L‑10] | Use SafeCast to downcast safely |
3 |
Total 117 instances over 46 issues:
| ID | Issue | Instances |
|---|---|---|
| [N‑01] | abi.encodePacked() should be replaced with bytes.concat() |
1 |
| [N‑02] | Import declarations should import specific identifiers, rather than the whole file | 4 |
| [N‑03] | Too long functions should be refactored | 1 |
| [N‑04] | There is no need to initialize variables with 0 | 1 |
| [N‑05] | Names of private/internal functions should be prefixed with an underscore |
5 |
| [N‑06] | Order of functions does not follow the Solidity Style Guide | 1 |
| [N‑07] | Custom errors should be used rather than revert()/require() |
2 |
| [N‑08] | Assembly blocks should have extensive comments | 1 |
| [N‑09] | assert() should be replaced with require() or revert() |
1 |
| [N‑10] | Complex casting | 5 |
| [N‑11] | Contracts should each be defined in separate files | 1 |
| [N‑12] | Events should be emitted before external calls | 1 |
| [N‑13] | Events are emitted without the sender information | 2 |
| [N‑14] | Event is missing indexed fields |
1 |
| [N‑15] | @openzeppelin/contracts should be upgraded to a newer version | 1 |
| [N‑16] | Magic numbers should be replaced with constants | 1 |
| [N‑17] | Contracts containing only utility functions should be made into libraries | 1 |
| [N‑18] | Contracts should have NatSpec @author tags |
2 |
| [N‑19] | Contracts should have NatSpec @title tags |
2 |
| [N‑20] | Event declarations should have NatSpec descriptions | 2 |
| [N‑21] | NatSpec documentation for function is missing | 8 |
| [N‑22] | Functions missing NatSpec @param tag |
15 |
| [N‑23] | Public variable declarations should have NatSpec descriptions | 1 |
| [N‑24] | Functions missing NatSpec @return tag |
3 |
| [N‑25] | Variables should be named in mixedCase style | 1 |
| [N‑26] | Non-assembly method available | 1 |
| [N‑27] | Missing zero address check in functions with address parameters | 13 |
| [N‑28] | Named imports of parent contracts are missing | 2 |
| [N‑29] | Constants should be put on the left side of comparisons | 1 |
| [N‑30] | Non-interface files should use fixed compiler versions | 1 |
| [N‑31] | High cyclomatic complexity | 1 |
| [N‑32] | Consider bounding input array length | 1 |
| [N‑33] | Use the latest Solidity version (0.8.19 for L2s) | 1 |
| [N‑34] | Use uint256/int256 instead of uint/int |
2 |
| [N‑35] | Functions with array parameters should have length checks in place | 3 |
| [N‑36] | Contract functions should use an interface |
2 |
| [N‑37] | Control structures do not follow the Solidity Style Guide | 1 |
| [N‑38] | Missing event for critical changes | 1 |
| [N‑39] | Consider adding emergency-stop functionality | 1 |
| [N‑40] | Consider adding a block/deny-list | 2 |
| [N‑41] | Enable IR-based code generation | 1 |
| [N‑42] | Functions should have @notice tags |
8 |
| [N‑43] | Large or complicated code bases should implement invariant tests | 1 |
| [N‑44] | Top-level declarations should be separated by at least two lines | 8 |
| [N‑45] | Consider adding formal verification proofs | 1 |
| [N‑46] | Prevent re-setting a state variable with the same value | 1 |
Total 46 instances over 25 issueswith 5144 gas saved:
| ID | Issue | Instances | Gas |
|---|---|---|---|
| [G‑01] | Constructors can be marked as payable to save deployment gas | 2 | 42 |
| [G‑02] | State variables that are never modified after deployment/constructor should be declared as constant or immutable |
1 | 2097 |
| [G‑03] | Using ++X/--Xinstead ofX++/X--` can save gas |
1 | 5 |
| [G‑04] | Use custom errors rather than revert()/require() strings to save gas |
2 | 100 |
| [G‑05] | State variables that are used multiple times in a function should be cached in stack variables | 7 | 679 |
| [G‑06] | internal functions only called once can be inlined to save gas |
6 | 180 |
| [G‑07] | Using this to access functions results in an external call, wasting gas |
1 | 100 |
| [G‑08] | Functions that revert when called by normal users can be marked payable |
1 | 21 |
| [G‑09] | Operator >=/<= costs less gas than operator >/< |
4 | 12 |
| [G‑10] | require()/revert() strings longer than 32 bytes cost extra gas |
2 | 6 |
| [G‑11] | Increments can be unchecked to save gas |
1 | 60 |
| [G‑12] | Using assembly to check for zero can save gas | 1 | 6 |
| [G‑13] | Use assembly to write address/contract type storage values |
1 | 50 |
| [G‑14] | Use != 0 or == 0 for unsigned integer comparison |
2 | 8 |
| [G‑15] | Avoid contract existence checks by using low level calls | 1 | 100 |
| [G‑16] | Avoid zero transfer to save gas | 3 | 300 |
| [G‑17] | Optimize names to save gas | 1 | 22 |
| [G‑18] | Reduce gas usage by moving to Solidity 0.8.19 or later | 1 | - |
| [G‑19] | Newer versions of solidity are more gas efficient | 1 | - |
| [G‑20] | Avoid updating storage when the value hasn't changed | 1 | 800 |
| [G‑21] | The result of a function call should be cached rather than re-calling the function | 1 | 100 |
| [G‑22] | Use assembly to emit events | 2 | 76 |
| [G‑23] | Use assembly to compute hashes to save gas | 1 | 80 |
| [G‑24] | Use calldata instead of memory for immutable arguments |
1 | 300 |
| [G‑25] | Consider activating via-ir for deploying | 1 | - |
The issues below may be reported by other bots/wardens, but can be penalized/ignored since either the rule or the specified instances are invalid.
Total 52 instances over 29 issues:
| ID | Issue | Instances |
|---|---|---|
| [D‑01] | abi.encodePacked() should be replaced with bytes.concat() |
1 |
| [D‑02] | 2 | |
| [D‑03] | bytes or bytes32 for clearer semantic meaning |
2 |
| [D‑04] | approve() not checked |
1 |
| [D‑05] | indexed fields |
1 |
| [D‑06] | abi.encodePacked() with dynamic arguments to a hash can cause collisions |
1 |
| [D‑07] | @notice tags |
2 |
| [D‑08] | 1 | |
| [D‑09] | 1 | |
| [D‑10] | transfer()/transferFrom() not checked |
3 |
| [D‑11] | 1 | |
| [D‑12] | != 0 or == 0 for unsigned integer comparison |
2 |
| [D‑13] | 6 | |
| [D‑14] | 1 | |
| [D‑15] | safeTransfer function does not check for contract existence |
2 |
| [D‑16] | 1 | |
| [D‑17] | 3 | |
| [D‑18] | 1 | |
| [D‑19] | 1 | |
| [D‑20] | 2 | |
| [D‑21] | require() or revert() statements that check input arguments should be at the top of the function |
1 |
| [D‑22] | 3 | |
| [D‑23] | SafeCast to downcast safely |
1 |
| [D‑24] | transfer()/transferFrom() |
3 |
| [D‑25] | 1 | |
| [D‑26] | 1 | |
| [D‑27] | 1 | |
| [D‑28] | 3 | |
| [D‑29] | 3 |
Contracts with privileged functions need owner/admin to be trusted not to perform malicious updates or drain funds. This may also cause a single point failure.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L151 ):
151: function setUri(string memory uri) external onlyOwner {Ownable2Step and Ownable2StepUpgradeable prevent the contract ownership from mistakenly being transferred to an address that cannot handle it (e.g. due to a typo in the address), by requiring that the recipient of the owner permissions actively accept via a contract call of its own.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L25 ):
25: contract ERC20MultiDelegate is ERC1155, Ownable {Constructors often take address parameters to initialize important components of a contract, such as owner or linked contracts. However, without a checking, there's a risk that an address parameter could be mistakenly set to the zero address (0x0). This could be due to an error 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 will be irretrievable. It's therefore 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.
There are 3 instances:
/// @audit `_token not checked`
/// @audit `_delegate not checked`
16: constructor(ERC20Votes _token, address _delegate) {
/// @audit `_token not checked`
44: constructor(
45: ERC20Votes _token,
46: string memory _metadata_uri
47: ) ERC1155(_metadata_uri) {Solidity version 0.8.20 or above uses the new Shanghai EVM which introduces the PUSH0 opcode. This op code may not yet be implemented on all evm-chains or Layer2s, so deployment on these chains will fail. Consider using an earlier solidity version.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L2 ):
2: pragma solidity ^0.8.2;This project is using specific package versions which are vulnerable to the specific CVEs listed below. Consider switching to more recent versions of these packages that don't have these vulnerabilities.
-
CVE-2022-39384 - MEDIUM - (
@openzeppelin/contracts >=3.2.0 <4.4.1): OpenZeppelin Contracts is a library for secure smart contract development. Before version 4.4.1 but after 3.2.0, initializer functions that are invoked separate from contract creation (the most prominent example being minimal proxies) may be reentered if they make an untrusted non-view external call. Once an initializer has finished running it can never be re-executed. However, an exception put in place to support multiple inheritance made reentrancy possible in the scenario described above, breaking the expectation that there is a single execution. Note that upgradeable proxies are commonly initialized together with contract creation, where reentrancy is not feasible, so the impact of this issue is believed to be minor. This issue has been patched, please upgrade to version 4.4.1. As a workaround, avoid untrusted external calls during initialization. -
CVE-2022-35961 - MEDIUM - (
@openzeppelin/contracts >=4.1.0 <4.7.3): OpenZeppelin Contracts is a library for secure smart contract development. The functionsECDSA.recoverandECDSA.tryRecoverare vulnerable to a kind of signature malleability due to accepting EIP-2098 compact signatures in addition to the traditional 65 byte signature format. This is only an issue for the functions that take a singlebytesargument, and not the functions that taker, v, sorr, vsas separate arguments. The potentially affected contracts are those that implement signature reuse or replay protection by marking the signature itself as used rather than the signed message or a nonce included in it. A user may take a signature that has already been submitted, submit it again in a different form, and bypass this protection. The issue has been patched in 4.7.3. -
CVE-2022-35915 - MEDIUM - (
@openzeppelin/contracts >=2.0.0 <4.7.2): OpenZeppelin Contracts is a library for secure smart contract development. The target contract of an EIP-165supportsInterfacequery can cause unbounded gas consumption by returning a lot of data, while it is generally assumed that this operation has a bounded cost. The issue has been fixed in v4.7.2. Users are advised to upgrade. There are no known workarounds for this issue. -
CVE-2022-31198 - HIGH - (
@openzeppelin/contracts >=4.3.0 <4.7.2): OpenZeppelin Contracts is a library for secure smart contract development. This issue concerns instances of Governor that use the moduleGovernorVotesQuorumFraction, a mechanism that determines quorum requirements as a percentage of the voting token's total supply. In affected instances, when a proposal is passed to lower the quorum requirements, past proposals may become executable if they had been defeated only due to lack of quorum, and the number of votes it received meets the new quorum requirement. Analysis of instances on chain found only one proposal that met this condition, and we are actively monitoring for new occurrences of this particular issue. This issue has been patched in v4.7.2. Users are advised to upgrade. Users unable to upgrade should consider avoiding lowering quorum requirements if a past proposal was defeated for lack of quorum. -
CVE-2022-31172 - HIGH - (
@openzeppelin/contracts >=4.1.0 <4.7.1): OpenZeppelin Contracts is a library for smart contract development. Versions 4.1.0 until 4.7.1 are vulnerable to the SignatureChecker reverting.SignatureChecker.isValidSignatureNowis not expected to revert. However, an incorrect assumption about Solidity 0.8'sabi.decodeallows some cases to revert, given a target contract that doesn't implement EIP-1271 as expected. The contracts that may be affected are those that useSignatureCheckerto check the validity of a signature and handle invalid signatures in a way other than reverting. The issue was patched in version 4.7.1. -
CVE-2022-31170 - HIGH - (
@openzeppelin/contracts >=4.0.0 <4.7.1): OpenZeppelin Contracts is a library for smart contract development. Versions 4.0.0 until 4.7.1 are vulnerable to ERC165Checker reverting instead of returningfalse.ERC165Checker.supportsInterfaceis designed to always successfully return a boolean, and under no circumstance revert. However, an incorrect assumption about Solidity 0.8'sabi.decodeallows some cases to revert, given a target contract that doesn't implement EIP-165 as expected, specifically if it returns a value other than 0 or 1. The contracts that may be affected are those that useERC165Checkerto check for support for an interface and then handle the lack of support in a way other than reverting. The issue was patched in version 4.7.1.
There are 6 instances:
- Global finding
There is 1 instance:
- ERC20MultiDelegate.sol ( #L48 ):
48: token = _token;A variable declaration shadowing any other existing definition is error-prone. It can cause confusion for developers, potentially introduce bugs, and reduce the readability and maintainability.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L151 ):
/// @audit Shadows `function uri()`
151: function setUri(string memory uri) external onlyOwner {Each of the following contracts implements or inherits the renounceOwnership() function. This can represent a certain risk if the ownership is renounced for any other reason than by design. Renouncing ownership will leave the contract without an owner, thereby removing any functionality that is only available to the owner.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L25 ):
25: contract ERC20MultiDelegate is ERC1155, Ownable {Constructors and initialization functions play a critical role in contracts by setting important initial states when the contract is first deployed before the system starts. The parameters passed to the constructor and initialization functions directly affect the behavior of the contract / protocol. If incorrect parameters are provided, the system may fail to run, behave abnormally, be unstable, or lack security. Therefore, it's crucial to carefully check each parameter in the constructor and initialization functions. If an exception is found, the transaction should be rolled back.
There are 3 instances:
/// @audit `_token`
/// @audit `_delegate`
16: constructor(ERC20Votes _token, address _delegate) {
/// @audit `_token`
44: constructor(
45: ERC20Votes _token,
46: string memory _metadata_uri
47: ) ERC1155(_metadata_uri) {Calling external functions within loops can easily result in insufficient gas. This greatly increases the likelihood of transaction failures, DOS attacks, and other unexpected actions. It is recommended to limit the number of loops within loops that call external functions, and to limit the gas line for each external call.
There are 2 instances:
/// @audit Calling `_reimburse()` within `for` loop, it will trigger an external call - `transferFrom()`
103: _reimburse(source, amount);
/// @audit Calling `createProxyDelegatorAndTransfer()` within `for` loop, it will trigger an external call - `transferFrom()`
106: createProxyDelegatorAndTransfer(target, amount);When a type is downcast to a smaller type, the higher order bits are truncated, effectively applying a modulo to the original value. The loss of data may cause incorrect calculations, unexpected state changes, or other unexpected behavior. It is recommended to use the SafeCast library.
There are 3 instances:
/// @audit uint256 -> uint160
91: ? address(uint160(sources[transferIndex]))
/// @audit uint256 -> uint160
94: ? address(uint160(targets[transferIndex]))
/// @audit uint256 -> uint160
214: return address(uint160(uint256(hash)));Solidity version 0.8.4 introduces bytes.concat(), which can be used to replace abi.encodePacked() on bytes/strings. It can make the intended operation clearer, leading to less reviewer confusion.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L202-L205 ):
202: bytes memory bytecode = abi.encodePacked(
203: type(ERC20ProxyDelegator).creationCode,
204: abi.encode(_token, _delegate)
205: );Using import declarations of the form import {<identifier_name>} from "some/file.sol" avoids polluting the symbol namespace making flattened files smaller, and speeds up compilation (but does not save any gas).
There are 4 instances:
6: import "@openzeppelin/contracts/access/Ownable.sol";
7: import "@openzeppelin/contracts/token/ERC1155/ERC1155.sol";
8: import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Votes.sol";
9: import "@openzeppelin/contracts/utils/math/Math.sol";Functions with too many lines are difficult to understand. It is recommended to refactor complex functions into multiple shorter and easier to understand functions.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L65 ):
/// @audit 52 lines
65: function _delegateMulti(Since the variables are automatically set to 0 when created, it is redundant to initialize it with 0 again.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L86 ):
86: uint transferIndex = 0;It is recommended by the Solidity Style Guide.
There are 5 instances:
- ERC20MultiDelegate.sol ( #L155-L158, #L163-L167, #L173-L175, #L192-L194, #L198-L201 ):
155: function createProxyDelegatorAndTransfer(
156: address target,
157: uint256 amount
158: ) internal {
163: function transferBetweenDelegators(
164: address from,
165: address to,
166: uint256 amount
167: ) internal {
173: function deployProxyDelegatorIfNeeded(
174: address delegate
175: ) internal returns (address) {
192: function getBalanceForDelegate(
193: address delegate
194: ) internal view returns (uint256) {
198: function retrieveProxyContractAddress(
199: ERC20Votes _token,
200: address _delegate
201: ) private view returns (address) {According to the Solidity Style Guide, functions should be grouped according to their visibility and ordered: constructor, receive, fallback, external, public, internal, private. Within a grouping, place the view and pure functions last.
There is 1 instance:
/// @audit ↓↓ Out of order with `setUri()`
144: function _reimburse(address source, uint256 amount) internal {
/// @audit ↑↑ Out of order with `_reimburse()`
151: function setUri(string memory uri) external onlyOwner {Custom errors are available from solidity version 0.8.4. Custom errors are more easily processed in try-catch blocks, and are easier to re-use and maintain.
There are 2 instances:
74: require(
75: sourcesLength > 0 || targetsLength > 0,
76: "Delegate: You should provide at least one source or one target delegate"
77: );
79: require(
80: Math.max(sourcesLength, targetsLength) == amountsLength,
81: "Delegate: The number of amounts must be equal to the greater of the number of sources or targets"
82: );Assembly blocks take a lot more time to audit than normal Solidity code, and often have gotchas and side-effects that the Solidity versions of the same code do not. Consider adding more comments explaining what is being done in every step of the assembly code, and describe why assembly is being used instead of Solidity.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L180-L182 ):
180: assembly {
181: bytecodeSize := extcodesize(proxyAddress)
182: }In versions of Solidity prior to 0.8.0, when encountering an assert all the remaining gas will be consumed. Even after solidity 0.8.0, the assert function is still not recommended, as described in the documentation:
Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L131 ):
131: assert(amount <= balance);Consider whether the number of casts is really necessary, or whether using a different type would be more appropriate. Alternatively, add comments to explain in detail why the casts are necessary, and any implicit reasons why the cast does not introduce an overflow.
There are 5 instances:
91: ? address(uint160(sources[transferIndex]))
94: ? address(uint160(targets[transferIndex]))
195: return ERC1155(this).balanceOf(msg.sender, uint256(uint160(delegate)));
214: return address(uint160(uint256(hash)));
214: return address(uint160(uint256(hash)));Keeping each contract in a separate file makes it easier to work with multiple people, makes the code easier to maintain, and is a common practice on most projects. The following files each contains more than one contract/library/interface.
There is 1 instance:
Ensure that events follow the best practice of check-effects-interaction, and are emitted before external calls.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L136 ):
/// @audit `deployProxyDelegatorIfNeeded()` is called on line 133
136: emit DelegationProcessed(source, target, amount);When an action is triggered based on a user's action, not being able to filter based on who triggered the action makes event processing a lot more cumbersome. Including the msg.sender the events of these types of action will make events much more useful to end users, especially when msg.sender is not tx.origin.
There are 2 instances:
136: emit DelegationProcessed(source, target, amount);
187: emit ProxyDeployed(delegate, proxyAddress);Index event fields makes the field more quickly accessible to off-chain tools that parse events. However, note that each indexed field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L32 ):
32: event ProxyDeployed(address indexed delegate, address proxyAddress);These contracts import contracts from @openzeppelin/contracts, but they are not using the latest version.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L4 ):
4: import {Address} from "@openzeppelin/contracts/utils/Address.sol";Magic numbers are hard-coded values in code that can make it difficult for developers and maintainers to understand the code, and can also cause confusion or errors. To improve the readability and maintainability of code, it is recommended to replace magic numbers with constants that have good readability.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L208 ):
/// @audit 0xff
208: bytes1(0xff),There is 1 instance:
- ERC20MultiDelegate.sol ( #L15 ):
15: contract ERC20ProxyDelegator {There are 2 instances:
15: contract ERC20ProxyDelegator {
25: contract ERC20MultiDelegate is ERC1155, Ownable {Some contract definitions have an incomplete NatSpec: add a @title notation to describe the contract to improve the code documentation.
There are 2 instances:
15: contract ERC20ProxyDelegator {
25: contract ERC20MultiDelegate is ERC1155, Ownable {There are 2 instances:
32: event ProxyDeployed(address indexed delegate, address proxyAddress);
33: event DelegationProcessed(
34: address indexed from,
35: address indexed to,
36: uint256 amount
37: );It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as DeFi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.
There are 8 instances:
16: constructor(ERC20Votes _token, address _delegate) {
65: function _delegateMulti(
151: function setUri(string memory uri) external onlyOwner {
155: function createProxyDelegatorAndTransfer(
163: function transferBetweenDelegators(
173: function deployProxyDelegatorIfNeeded(
192: function getBalanceForDelegate(
198: function retrieveProxyContractAddress(There are 15 instances (click to show):
- ERC20MultiDelegate.sol ( #L16, #L65-L69, #L151, #L155-L158, #L163-L167, #L173-L175, #L192-L194, #L198-L201 ):
/// @audit Missing @param for `_token`, `_delegate`
16: constructor(ERC20Votes _token, address _delegate) {
/// @audit Missing @param for `sources`, `targets`, `amounts`
65: function _delegateMulti(
66: uint256[] calldata sources,
67: uint256[] calldata targets,
68: uint256[] calldata amounts
69: ) internal {
/// @audit Missing @param for `uri`
151: function setUri(string memory uri) external onlyOwner {
/// @audit Missing @param for `target`, `amount`
155: function createProxyDelegatorAndTransfer(
156: address target,
157: uint256 amount
158: ) internal {
/// @audit Missing @param for `from`, `to`, `amount`
163: function transferBetweenDelegators(
164: address from,
165: address to,
166: uint256 amount
167: ) internal {
/// @audit Missing @param for `delegate`
173: function deployProxyDelegatorIfNeeded(
174: address delegate
175: ) internal returns (address) {
/// @audit Missing @param for `delegate`
192: function getBalanceForDelegate(
193: address delegate
194: ) internal view returns (uint256) {
/// @audit Missing @param for `_token`, `_delegate`
198: function retrieveProxyContractAddress(
199: ERC20Votes _token,
200: address _delegate
201: ) private view returns (address) {It is recommended to use the NatSpec tags @notice, @dev, @return, @inheritdoc for public state variables.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L28 ):
28: ERC20Votes public token;There are 3 instances:
- ERC20MultiDelegate.sol ( #L173-L175, #L192-L194, #L198-L201 ):
173: function deployProxyDelegatorIfNeeded(
174: address delegate
175: ) internal returns (address) {
192: function getBalanceForDelegate(
193: address delegate
194: ) internal view returns (uint256) {
198: function retrieveProxyContractAddress(
199: ERC20Votes _token,
200: address _delegate
201: ) private view returns (address) {As the Solidity Style Guide suggests: arguments, local variables and mutable state variables should be named in mixedCase style.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L46 ):
/// @audit _metadata_uri
46: string memory _metadata_uriThere are some automated tools that will flag a project as having higher complexity if there is inline-assembly, so it's best to avoid using it where it's not necessary. In addition, most assembly methods can be replaced by non-assembly methods, for example:
assembly{ g := gas() }=>uint256 g = gasleft()assembly{ id := chainid() }=>uint256 id = block.chainidassembly { r := mulmod(a, b, d) }=>uint256 m = mulmod(x, y, k)assembly { size := extcodesize() }=>uint256 size = address(a).code.length- etc.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L181 ):
181: bytecodeSize := extcodesize(proxyAddress)Adding a zero address check for each address type parameter can prevent errors.
There are 13 instances (click to show):
- ERC20MultiDelegate.sol ( #L16, #L44-L47, #L124-L128, #L144, #L155-L158, #L163-L167, #L173-L175, #L192-L194, #L198-L201 ):
/// @audit `_token not checked`
/// @audit `_delegate not checked`
16: constructor(ERC20Votes _token, address _delegate) {
/// @audit `_token not checked`
44: constructor(
45: ERC20Votes _token,
46: string memory _metadata_uri
47: ) ERC1155(_metadata_uri) {
/// @audit `source not checked`
/// @audit `target not checked`
124: function _processDelegation(
125: address source,
126: address target,
127: uint256 amount
128: ) internal {
/// @audit `source not checked`
144: function _reimburse(address source, uint256 amount) internal {
/// @audit `target not checked`
155: function createProxyDelegatorAndTransfer(
156: address target,
157: uint256 amount
158: ) internal {
/// @audit `from not checked`
/// @audit `to not checked`
163: function transferBetweenDelegators(
164: address from,
165: address to,
166: uint256 amount
167: ) internal {
/// @audit `delegate not checked`
173: function deployProxyDelegatorIfNeeded(
174: address delegate
175: ) internal returns (address) {
/// @audit `delegate not checked`
192: function getBalanceForDelegate(
193: address delegate
194: ) internal view returns (uint256) {
/// @audit `_token not checked`
/// @audit `_delegate not checked`
198: function retrieveProxyContractAddress(
199: ERC20Votes _token,
200: address _delegate
201: ) private view returns (address) {There are 2 instances:
- ERC20MultiDelegate.sol ( #L25 ):
/// @audit ERC1155
/// @audit Ownable
25: contract ERC20MultiDelegate is ERC1155, Ownable {Putting constants on the left side of comparison statements is a best practice known as Yoda conditions. Although solidity's static typing system prevents accidental assignments within conditionals, adopting this practice can improve code readability and consistency, especially when working across multiple languages.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L185 ):
/// @audit put `0` on the left
185: if (bytecodeSize == 0) {To prevent the actual contracts being deployed from behaving differently depending on the compiler version, it is recommended to use fixed solidity versions for contracts and libraries.
Although we can configure a specific version through config (like hardhat, forge config files), it is recommended to set the fixed version in the solidity pragma directly before deploying to the mainnet.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L2 ):
2: pragma solidity ^0.8.2;Consider breaking down these blocks into more manageable units, by splitting things into utility functions, by reducing nesting, and by using early returns.
There is 1 instance (click to show):
- ERC20MultiDelegate.sol ( #L65-L116 ):
65: function _delegateMulti(
66: uint256[] calldata sources,
67: uint256[] calldata targets,
68: uint256[] calldata amounts
69: ) internal {
70: uint256 sourcesLength = sources.length;
71: uint256 targetsLength = targets.length;
72: uint256 amountsLength = amounts.length;
73:
74: require(
75: sourcesLength > 0 || targetsLength > 0,
76: "Delegate: You should provide at least one source or one target delegate"
77: );
78:
79: require(
80: Math.max(sourcesLength, targetsLength) == amountsLength,
81: "Delegate: The number of amounts must be equal to the greater of the number of sources or targets"
82: );
83:
84: // Iterate until all source and target delegates have been processed.
85: for (
86: uint transferIndex = 0;
87: transferIndex < Math.max(sourcesLength, targetsLength);
88: transferIndex++
89: ) {
...... OMITTED ......
92: : address(0);
93: address target = transferIndex < targetsLength
94: ? address(uint160(targets[transferIndex]))
95: : address(0);
96: uint256 amount = amounts[transferIndex];
97:
98: if (transferIndex < Math.min(sourcesLength, targetsLength)) {
99: // Process the delegation transfer between the current source and target delegate pair.
100: _processDelegation(source, target, amount);
101: } else if (transferIndex < sourcesLength) {
102: // Handle any remaining source amounts after the transfer process.
103: _reimburse(source, amount);
104: } else if (transferIndex < targetsLength) {
105: // Handle any remaining target amounts after the transfer process.
106: createProxyDelegatorAndTransfer(target, amount);
107: }
108: }
109:
110: if (sourcesLength > 0) {
111: _burnBatch(msg.sender, sources, amounts[:sourcesLength]);
112: }
113: if (targetsLength > 0) {
114: _mintBatch(msg.sender, targets, amounts[:targetsLength], "");
115: }
116: }The functions below take in an unbounded array, and make function calls for entries in the array. While the function will revert if it eventually runs out of gas, it may be a nicer user experience to require() that the length of the array is below some reasonable maximum, so that the user doesn't have to use up a full transaction's gas only to see that the transaction reverts.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L85 ):
85: for (Upgrading to the latest solidity version can optimize gas usage, take advantage of new features and improve overall contract efficiency. Where possible, based on compatibility requirements, it is recommended to use newer/latest solidity version to take advantage of the latest optimizations and features.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L2 ):
2: pragma solidity ^0.8.2;It is recommended to use uint256/int256 instead of uint/int in function parameters, since they are used for function signatures.
There are 2 instances:
/// @audit uint transferIndex
86: uint transferIndex = 0;
/// @audit uint bytecodeSize
179: uint bytecodeSize;There are 3 instances:
- ERC20MultiDelegate.sol ( #L57-L60 ):
/// @audit sources
/// @audit targets
/// @audit amounts
57: function delegateMulti(
58: uint256[] calldata sources,
59: uint256[] calldata targets,
60: uint256[] calldata amountsAll external/public functions should extend an interface. This is useful to ensure that the whole API is extracted and can be more easily integrated by other projects.
There are 2 instances:
57: function delegateMulti(
58: uint256[] calldata sources,
59: uint256[] calldata targets,
60: uint256[] calldata amounts
61: ) external {
151: function setUri(string memory uri) external onlyOwner {Refer to the Solidity style guide - Control Structures.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L85 ):
85: for (Events should be emitted when critical changes are made to the contracts.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L151-L153 ):
151: function setUri(string memory uri) external onlyOwner {
152: _setURI(uri);
153: }Adding a way to quickly halt protocol functionality in an emergency, rather than having to pause individual contracts one-by-one, will make in-progress hack mitigation faster and much less stressful.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L25 ):
25: contract ERC20MultiDelegate is ERC1155, Ownable {Doing so will significantly increase centralization, but will help to prevent hackers from using stolen tokens
There are 2 instances:
15: contract ERC20ProxyDelegator {
25: contract ERC20MultiDelegate is ERC1155, Ownable {The IR-based code generator was introduced with an aim to not only allow code generation to be more transparent and auditable but also to enable more powerful optimization passes that span across functions.
You can enable it on the command line using --via-ir or with the option {"viaIR": true}.
This will take longer to compile, but you can just simple test it before deploying and if you got a better benchmark then you can add --via-ir to your deploy command
More on: https://docs.soliditylang.org/en/v0.8.17/ir-breaking-changes.html
There is 1 instance:
- Global finding
The @notice is used to explain to users what the function does. The compiler interprets /// or /** comments as this tag if one wasn't explicitly provided.
There are 8 instances:
16: constructor(ERC20Votes _token, address _delegate) {
65: function _delegateMulti(
151: function setUri(string memory uri) external onlyOwner {
155: function createProxyDelegatorAndTransfer(
163: function transferBetweenDelegators(
173: function deployProxyDelegatorIfNeeded(
192: function getBalanceForDelegate(
198: function retrieveProxyContractAddress(This includes: large code bases, or code with lots of inline-assembly, complicated math, or complicated interactions between multiple contracts. Invariant fuzzers such as Echidna require the test writer to come up with invariants which should not be violated under any circumstances, and the fuzzer tests various inputs and function calls to ensure that the invariants always hold. Even code with 100% code coverage can still have bugs due to the order of the operations a user performs, and invariant fuzzers may help significantly.
There is 1 instance:
- Global finding
There are 8 instances (click to show):
- ERC20MultiDelegate.sol ( #L2-L4, #L63-L65, #L149-L151, #L153-L155, #L161-L163, #L171-L173, #L190-L192, #L196-L198 ):
2: pragma solidity ^0.8.2;
3:
4: import {Address} from "@openzeppelin/contracts/utils/Address.sol";
63: }
64:
65: function _delegateMulti(
149: }
150:
151: function setUri(string memory uri) external onlyOwner {
153: }
154:
155: function createProxyDelegatorAndTransfer(
161: }
162:
163: function transferBetweenDelegators(
171: }
172:
173: function deployProxyDelegatorIfNeeded(
190: }
191:
192: function getBalanceForDelegate(
196: }
197:
198: function retrieveProxyContractAddress(Formal verification is the act of proving or disproving the correctness of intended algorithms underlying a system with respect to a certain formal specification/property/invariant, using formal methods of mathematics.
Some tools that are currently available to perform these tests on smart contracts are SMTChecker and Certora Prover.
There is 1 instance:
This especially problematic when the setter also emits the same value, which may be confusing to offline parsers.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L152 ):
152: _setURI(uri);Payable functions cost less gas to execute, because the compiler does not have to add extra checks to ensure that no payment is provided. A constructor can be safely marked as payable, because only the deployer would be able to pass funds, and the project itself would not pass any funds.
There are 2 instances:
16: constructor(ERC20Votes _token, address _delegate) {
44: constructor(
45: ERC20Votes _token,
46: string memory _metadata_uri
47: ) ERC1155(_metadata_uri) {[G‑02] State variables that are never modified after deployment/constructor should be declared as constant or immutable
This can avoid a Gsset (20000 gas) on deployment (in constructor), and replaces the first access in each transaction (Gcoldsload - 2100 gas) and each access thereafter (Gwarmacces - 100 gas) with a PUSH32 (3 gas).
While strings are not value types, and therefore cannot be immutable/constant if not hard-coded outside of the constructor, the same behavior can be achieved by making the current contract abstract with virtual functions for the string accessors, and having a child contract override the functions with the hard-coded implementation-specific values.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L28 ):
28: ERC20Votes public token;It can save 5 gas for each execution / per iteration.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L88 ):
88: transferIndex++Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas.
There are 2 instances:
74: require(
75: sourcesLength > 0 || targetsLength > 0,
76: "Delegate: You should provide at least one source or one target delegate"
77: );
79: require(
80: Math.max(sourcesLength, targetsLength) == amountsLength,
81: "Delegate: The number of amounts must be equal to the greater of the number of sources or targets"
82: );[G‑05] State variables that are used multiple times in a function should be cached in stack variables
When performing multiple operations on a state variable in a function, it is recommended to cache it first. Either multiple reads or multiple writes to a state variable can save gas by caching it on the stack. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses. Saves 100 gas per instance.
There are 7 instances:
/// @audit token: 2 reads
144: function _reimburse(address source, uint256 amount) internal {
/// @audit token: 3 reads
163: function transferBetweenDelegators(
/// @audit token: 2 reads
173: function deployProxyDelegatorIfNeeded(If an internal function is only used once, there is no need to modularize it, unless the function calling it would otherwise be too long and complex. Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 6 instances (click to show):
- ERC20MultiDelegate.sol ( #L65-L69, #L124-L128, #L144, #L155-L158, #L163-L167, #L192-L194 ):
65: function _delegateMulti(
66: uint256[] calldata sources,
67: uint256[] calldata targets,
68: uint256[] calldata amounts
69: ) internal {
124: function _processDelegation(
125: address source,
126: address target,
127: uint256 amount
128: ) internal {
144: function _reimburse(address source, uint256 amount) internal {
155: function createProxyDelegatorAndTransfer(
156: address target,
157: uint256 amount
158: ) internal {
163: function transferBetweenDelegators(
164: address from,
165: address to,
166: uint256 amount
167: ) internal {
192: function getBalanceForDelegate(
193: address delegate
194: ) internal view returns (uint256) {External calls have an overhead of 100 gas, which can be avoided by not referencing the function using this. Contracts are allowed to override their parents' functions and change the visibility from external to public, so make this change if it's required in order to call the function internally.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L195 ):
195: return ERC1155(this).balanceOf(msg.sender, uint256(uint160(delegate)));If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.
The extra opcodes avoided are:
CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2)
which cost an average of about 21 gas per call to the function, in addition to the extra deployment cost.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L151 ):
151: function setUri(string memory uri) external onlyOwner {The compiler uses opcodes GT and ISZERO for code that uses >, but only requires LT for >=. A similar behavior applies for >, which uses opcodes LT and ISZERO, but only requires GT for <=. It can save 3 gas for each.
It should be converted to the <=/>= equivalent when comparing against integer literals.
There are 4 instances:
75: sourcesLength > 0 || targetsLength > 0,
75: sourcesLength > 0 || targetsLength > 0,
110: if (sourcesLength > 0) {
113: if (targetsLength > 0) {Each extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas
There are 2 instances:
74: require(
75: sourcesLength > 0 || targetsLength > 0,
76: "Delegate: You should provide at least one source or one target delegate"
77: );
79: require(
80: Math.max(sourcesLength, targetsLength) == amountsLength,
81: "Delegate: The number of amounts must be equal to the greater of the number of sources or targets"
82: );Using unchecked increments can save gas by bypassing the built-in overflow checks. This can save 30-40 gas per iteration. So it is recommended to use unchecked increments when overflow is not possible.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L85-L88 ):
85: for (
86: uint transferIndex = 0;
87: transferIndex < Math.max(sourcesLength, targetsLength);
88: transferIndex++Using assembly to check for zero can save gas by allowing more direct access to the evm and reducing some of the overhead associated with high-level operations in solidity.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L185 ):
185: if (bytecodeSize == 0) {Using assembly { sstore(state.slot, addr) instead of state = addr can save gas.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L48 ):
48: token = _token;For uint comparison in require() statements prior to solidity 0.8.13, it is cheaper to use != 0 or == 0 instead of > 0 or <= 0 for unsigned integer comparison in.
There are 2 instances:
/// @audit Replace with `!= 0`
75: sourcesLength > 0 || targetsLength > 0,
/// @audit Replace with `!= 0`
75: sourcesLength > 0 || targetsLength > 0,Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external function calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value. Similar behavior can be achieved in earlier versions by using low-level calls, since low level calls never check for contract existence.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L195 ):
195: return ERC1155(this).balanceOf(msg.sender, uint256(uint160(delegate)));In Solidity, unnecessary operations can waste gas. For example, a transfer function without a zero amount check uses gas even if called with a zero amount, since the contract state remains unchanged. Implementing a zero amount check avoids these unnecessary function calls, saving gas and improving efficiency.
There are 3 instances:
148: token.transferFrom(proxyAddressFrom, msg.sender, amount);
160: token.transferFrom(msg.sender, proxyAddress, amount);
170: token.transferFrom(proxyAddressFrom, proxyAddressTo, amount);public/external function names and public member variable names can be optimized to save gas. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L25 ):
/// @audit delegateMulti(), setUri(), token
25: contract ERC20MultiDelegate is ERC1155, Ownable {Solidity version 0.8.19 introduced a number of gas optimizations, refer to the Solidity 0.8.19 Release Announcement for details.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L2 ):
2: pragma solidity ^0.8.2;The solidity language continues to pursue more efficient gas optimization schemes. Adopting a newer version of solidity can be more gas efficient.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L2 ):
2: pragma solidity ^0.8.2;Manipulating storage in solidity is gas-intensive. It can be optimized by avoiding unnecessary storage updates when the new value equals the existing value. If the old value is equal to the new value, not re-storing the value will avoid a Gsreset (2900 gas), potentially at the expense of a Gcoldsload (2100 gas) or a Gwarmaccess (100 gas).
There is 1 instance:
- ERC20MultiDelegate.sol ( #L152 ):
152: _setURI(uri);The function calls in solidity are expensive. If the same result of the same function calls are to be used several times, the result should be cached to reduce the gas consumption of repeated calls.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L65 ):
/// @audit `Math.max(sourcesLength,targetsLength)` called on lines: 80, 87
65: function _delegateMulti(To efficiently emit events, it's possible to utilize assembly by making use of scratch space and the free memory pointer. This approach has the advantage of potentially avoiding the costs associated with memory expansion.
However, it's important to note that in order to safely optimize this process, it is preferable to cache and restore the free memory pointer.
A good example of such practice can be seen in Solady's codebase.
There are 2 instances:
136: emit DelegationProcessed(source, target, amount);
187: emit ProxyDeployed(delegate, proxyAddress);If the arguments to the encode call can fit into the scratch space (two words or fewer), then it's more efficient to use assembly to generate the hash (80 gas):
keccak256(abi.encodePacked(x, y)) -> assembly {mstore(0x00, a); mstore(0x20, b); let hash := keccak256(0x00, 0x40); }
There is 1 instance:
- ERC20MultiDelegate.sol ( #L211 ):
211: keccak256(bytecode)Mark data types as calldata instead of memory where possible. This makes it so that the data is not automatically loaded into memory. If the data passed into the function does not need to be changed (like updating values in an array), it can be passed in as calldata. The one exception to this is if the argument must later be passed into another function that takes an argument that specifies memory storage.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L151 ):
/// @audit uri
151: function setUri(string memory uri) external onlyOwner {By using --via-ir or {"viaIR": true}, the compiler is able to use more advanced multi-function optimizations, for extra gas savings.
There is 1 instance:
Solidity version 0.8.4 introduces bytes.concat(), which can be used to replace abi.encodePacked() on bytes/strings. It can make the intended operation clearer, leading to less reviewer confusion.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L207-L212 ):
/// @audit abi.encodePacked() is clearer for non-bytes parameters.
207: abi.encodePacked(
208: bytes1(0xff),
209: address(this),
210: uint256(0), // salt
211: keccak256(bytecode)
212: )The instances below are already CamelCase (events are supposed to use CamelCase, not lowerCamelCase).
There are 2 instances:
32: event ProxyDeployed(address indexed delegate, address proxyAddress);
33: event DelegationProcessed(The rule is valid, but the following findings are invalid.
There are 2 instances:
- ERC20MultiDelegate.sol ( #L202-L205, #L207-L212 ):
202: bytes memory bytecode = abi.encodePacked(
203: type(ERC20ProxyDelegator).creationCode,
204: abi.encode(_token, _delegate)
205: );
207: abi.encodePacked(
208: bytes1(0xff),
209: address(this),
210: uint256(0), // salt
211: keccak256(bytecode)
212: )The following are for known contracts that will revert if they fail.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L17 ):
17: _token.approve(msg.sender, type(uint256).max);Index event fields makes the field more quickly accessible to off-chain tools that parse events. However, note that each indexed field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L33-L37 ):
33: event DelegationProcessed(
34: address indexed from,
35: address indexed to,
36: uint256 amount
37: );The cases below do not have multiple bytes/string arguments
There is 1 instance:
- ERC20MultiDelegate.sol ( #L206-L213 ):
206: bytes32 hash = keccak256(
207: abi.encodePacked(
208: bytes1(0xff),
209: address(this),
210: uint256(0), // salt
211: keccak256(bytecode)
212: )
213: );The @notice is used to explain to users what the contract does. The compiler interprets /// or /** comments as this tag if one wasn't explicitly provided.
There are 2 instances:
15: contract ERC20ProxyDelegator {
25: contract ERC20MultiDelegate is ERC1155, Ownable {This is only true for state variables, and does not save gas for local variables. The examples below are for local variables and therefore do not save gas, and are invalid.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L86 ):
86: uint transferIndex = 0;The following are not ERC721.mint() calls.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L114 ):
114: _mintBatch(msg.sender, targets, amounts[:targetsLength], "");The following are non-ERC20 transfers, or for known contracts that will revert if they fail.
There are 3 instances:
148: token.transferFrom(proxyAddressFrom, msg.sender, amount);
160: token.transferFrom(msg.sender, proxyAddress, amount);
170: token.transferFrom(proxyAddressFrom, proxyAddressTo, amount);There is 1 instance:
- ERC20MultiDelegate.sol ( #L1 ):
1: // SPDX-License-Identifier: MITOnly valid prior to solidity version 0.8.13, and only for require() statements, and at least one of those is not true for the examples below.
There are 2 instances:
110: if (sourcesLength > 0) {
113: if (targetsLength > 0) {Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external function calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value. Similar behavior can be achieved in earlier versions by using low-level calls, since low level calls never check for contract existence.
There are 6 instances:
/// @audit Return value not used.
17: _token.approve(msg.sender, type(uint256).max);
/// @audit Return value not used.
18: _token.delegate(_delegate);
/// @audit Return value not used.
148: token.transferFrom(proxyAddressFrom, msg.sender, amount);
/// @audit Return value not used.
160: token.transferFrom(msg.sender, proxyAddress, amount);
/// @audit Return value not used.
170: token.transferFrom(proxyAddressFrom, proxyAddressTo, amount);
/// @audit Return value not used.
186: new ERC20ProxyDelegator{salt: 0}(token, delegate);Accessing the elements of the array without checking or ensuring the validity of the access index in advance. It may result in an unexpected out-of-bounds error, or may result in missing elements when trying to traverse the entire array.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L57-L61 ):
57: function delegateMulti(
58: uint256[] calldata sources,
59: uint256[] calldata targets,
60: uint256[] calldata amounts
61: ) external {The examples below are either not token transfers, or are making high-level transfer()/transferFrom() calls (which check for contract existence), or are from a library that checks for contract existence.
There are 2 instances:
16: constructor(ERC20Votes _token, address _delegate) {
44: constructor(This assembly block does not call mstore(), so it's not possible to hit the bug here even if there are small future changes, so this doesn't seem low severity.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L180 ):
/// @audit `mstore` not involved
180: assembly {[D‑17] Functions calling contracts/addresses with transfer hooks should be protected by reentrancy guard
The rule is valid, but the following findings are invalid.
There are 3 instances:
148: token.transferFrom(proxyAddressFrom, msg.sender, amount);
160: token.transferFrom(msg.sender, proxyAddress, amount);
170: token.transferFrom(proxyAddressFrom, proxyAddressTo, amount);Governance functions (such as upgrading contracts, setting critical parameters) should be controlled using time locks to introduce a delay between a proposal and its execution. This gives users time to exit before a potentially dangerous or malicious operation is applied.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L151 ):
151: function setUri(string memory uri) external onlyOwner {Writing data to the free memory pointer without later updating the free memory pointer will cause there to be dirty bits at that memory location. Not updating the free memory pointer will make it harder for the optimizer to reason about whether the memory needs to be cleaned before use, which will lead to worse optimizations. Update the free memory pointer and annotate the block (assembly ("memory-safe") { ... }) to avoid this issue.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L180 ):
180: assembly {The function calls in solidity are expensive. If the same result of the same function calls are to be used several times, the result should be cached to reduce the gas consumption of repeated calls.
There are 2 instances:
/// @audit retrieveProxyContractAddress(token,from)
168: address proxyAddressFrom = retrieveProxyContractAddress(token, from);
/// @audit retrieveProxyContractAddress(token,to)
169: address proxyAddressTo = retrieveProxyContractAddress(token, to);[D‑21] require() or revert() statements that check input arguments should be at the top of the function
require() or revert() statements that check input arguments should be at the top of the functionThe rule is valid, but the following findings are invalid.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L79-L82 ):
/// @audit should check before line 70
79: require(
80: Math.max(sourcesLength, targetsLength) == amountsLength,
81: "Delegate: The number of amounts must be equal to the greater of the number of sources or targets"
82: );The following are for known contracts that will not revert on zero transfers.
There are 3 instances:
148: token.transferFrom(proxyAddressFrom, msg.sender, amount);
160: token.transferFrom(msg.sender, proxyAddress, amount);
170: token.transferFrom(proxyAddressFrom, proxyAddressTo, amount);The rule is valid, but the following findings are invalid.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L210 ):
/// @audit uint256(0)
210: uint256(0), // saltSome tokens do not implement the ERC20 standard properly. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the ERC20 specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20/ERC20, their function signatures do not match and therefore the calls made will revert.
It is recommended to use the SafeERC20's safeTransfer() and safeTransferFrom() from OpenZeppelin instead.
There are 3 instances:
148: token.transferFrom(proxyAddressFrom, msg.sender, amount);
160: token.transferFrom(msg.sender, proxyAddress, amount);
170: token.transferFrom(proxyAddressFrom, proxyAddressTo, amount);If the arguments to the encode call can fit into the scratch space (two words or fewer), then it's more efficient to use assembly to generate the hash (80 gas):
keccak256(abi.encodePacked(x, y)) -> assembly {mstore(0x00, a); mstore(0x20, b); let hash := keccak256(0x00, 0x40); }
There is 1 instance:
- ERC20MultiDelegate.sol ( #L206-L213 ):
206: bytes32 hash = keccak256(
207: abi.encodePacked(
208: bytes1(0xff),
209: address(this),
210: uint256(0), // salt
211: keccak256(bytecode)
212: )
213: );While 100% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.
There is 1 instance:
- Global finding
Passing 0 or 0x0 as a function argument can sometimes result in a security issue(e.g. passing zero as the slippage parameter). 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.
Consider using a constant variable with a descriptive name, so it's clear that the argument is intentionally being used, and for the right reasons.
There is 1 instance:
- ERC20MultiDelegate.sol ( #L207-L212 ):
207: abi.encodePacked(
208: bytes1(0xff),
209: address(this),
210: uint256(0), // salt
211: keccak256(bytecode)
212: )Tokens such as COMP or UNI will revert when an address' balance reaches type(uint96).max. Ensure that the calls below can be broken up into smaller batches if necessary.
There are 3 instances:
148: token.transferFrom(proxyAddressFrom, msg.sender, amount);
160: token.transferFrom(msg.sender, proxyAddress, amount);
170: token.transferFrom(proxyAddressFrom, proxyAddressTo, amount);Rebasing tokens are tokens that have each holder's balanceof() increase over time. Aave aTokens are an example of such tokens. If rebasing tokens are used, rewards accrue to the contract holding the tokens, and cannot be withdrawn by the original depositor. To address the issue, track 'shares' deposited on a pro-rata basis, and let shares be redeemed for their proportion of the current balance at the time of the withdrawal.
There are 3 instances:
- ERC20MultiDelegate.sol ( #L144, #L155-L158, #L163-L167 ):
144: function _reimburse(address source, uint256 amount) internal {
155: function createProxyDelegatorAndTransfer(
156: address target,
157: uint256 amount
158: ) internal {
163: function transferBetweenDelegators(
164: address from,
165: address to,
166: uint256 amount
167: ) internal {
For transparency, the sponsor responded in regards to M-01: