Skip to content

Instantly share code, notes, and snippets.

@code423n4
Created October 6, 2023 16:19
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save code423n4/33c1c8dad584e9eba48c38c69495fb59 to your computer and use it in GitHub Desktop.
Save code423n4/33c1c8dad584e9eba48c38c69495fb59 to your computer and use it in GitHub Desktop.
Best bot finding from 2023-10-ens-bot-findings

Winning bot race submission

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.

Report

Audit report for 2023-10-ens generated by Bot-Henry.

Note: There is a section for disputed findings below the usual findings sections.

Summary

Medium Issues

Total 1 instance over 1 issue:

ID Issue Instances
[M‑01] Centralization risk for privileged functions 1

Low Issues

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

Non Critical Issues

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

Gas Optimizations

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 -

Disputed Issues

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] Event names should use CamelCase 2
[D‑03] Cast to bytes or bytes32 for clearer semantic meaning 2
[D‑04] Return values of approve() not checked 1
[D‑05] Event is missing indexed fields 1
[D‑06] Passing abi.encodePacked() with dynamic arguments to a hash can cause collisions 1
[D‑07] NatSpec: Contract declarations should have @notice tags 2
[D‑08] Not initializing local variables to zero saves gas 1
[D‑09] safeMint should be used in place of mint 1
[D‑10] Return values of transfer()/transferFrom() not checked 3
[D‑11] SPDX identifier should be the in the first line of a solidity file 1
[D‑12] Use != 0 or == 0 for unsigned integer comparison 2
[D‑13] Avoid contract existence checks by using low level calls 6
[D‑14] Array length is not checked before access its index 1
[D‑15] safeTransfer function does not check for contract existence 2
[D‑16] Low level calls with solidity before 0.8.14 result in an optimiser bug 1
[D‑17] Functions calling contracts/addresses with transfer hooks should be protected by reentrancy guard 3
[D‑18] Governance functions should be controlled by time locks 1
[D‑19] Assembly block creates dirty bits 1
[D‑20] Calculations should be memoized rather than re-calculating them 2
[D‑21] require() or revert() statements that check input arguments should be at the top of the function 1
[D‑22] Some tokens may revert when zero value transfers are made 3
[D‑23] Use SafeCast to downcast safely 1
[D‑24] Unsafe use of ERC20 transfer()/transferFrom() 3
[D‑25] Use assembly to compute hashes to save gas 1
[D‑26] Contracts should have full test coverage 1
[D‑27] Use descriptive constant instead of 0 as a parameter 1
[D‑28] Some tokens may revert when large transfers are made 3
[D‑29] Contracts are vulnerable to rebasing accounting-related issues 3

Medium Issues

[M‑01] Centralization risk for privileged functions

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 {

Low Issues

[L‑01] Use Ownable2Step instead of Ownable

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 {

[L‑02] Missing zero address check in constructor

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) {

[L‑03] Solidity version 0.8.20 or above may not work on other chains due to PUSH0

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;

[L‑04] Vulnerable versions of packages are being used

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 functions ECDSA.recover and ECDSA.tryRecover are 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 single bytes argument, and not the functions that take r, v, s or r, vs as 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-165 supportsInterface query 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 module GovernorVotesQuorumFraction, 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.isValidSignatureNow is not expected to revert. However, an incorrect assumption about Solidity 0.8's abi.decode allows 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 use SignatureChecker to 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 returning false. ERC165Checker.supportsInterface is designed to always successfully return a boolean, and under no circumstance revert. However, an incorrect assumption about Solidity 0.8's abi.decode allows 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 use ERC165Checker to 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

[L‑05] Missing checks for address(0) when setting address state variables

There is 1 instance:

  • ERC20MultiDelegate.sol ( #L48 ):
48:         token = _token;

[L‑06] Variables shadowing other definitions

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 {

[L‑07] Owner can renounce Ownership

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 {

[L‑08] Constructor / initialization function lacks parameter validation

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) {

[L‑09] External function calls within loops

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);

[L‑10] Use SafeCast to downcast safely

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)));

Non Critical Issues

[N‑01] abi.encodePacked() should be replaced with bytes.concat()

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:

202:         bytes memory bytecode = abi.encodePacked(
203:             type(ERC20ProxyDelegator).creationCode, 
204:             abi.encode(_token, _delegate)
205:         );

[N‑02] Import declarations should import specific identifiers, rather than the whole file

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";

[N‑03] Too long functions should be refactored

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(

[N‑04] There is no need to initialize variables with 0

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;

[N‑05] Names of private/internal functions should be prefixed with an underscore

It is recommended by the Solidity Style Guide.

There are 5 instances:

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) {

[N‑06] Order of functions does not follow the Solidity Style Guide

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 {

[N‑07] Custom errors should be used rather than revert()/require()

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:         );

[N‑08] Assembly blocks should have extensive comments

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:

180:         assembly {
181:             bytecodeSize := extcodesize(proxyAddress)
182:         }

[N‑09] assert() should be replaced with require() or revert()

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);

[N‑10] Complex casting

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)));

[N‑11] Contracts should each be defined in separate files

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:

[N‑12] Events should be emitted before external calls

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);

[N‑13] Events are emitted without the sender information

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);

[N‑14] Event is missing indexed fields

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);

[N‑15] @openzeppelin/contracts should be upgraded to a newer version

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";

[N‑16] Magic numbers should be replaced with constants

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),

[N‑17] Contracts containing only utility functions should be made into libraries

There is 1 instance:

  • ERC20MultiDelegate.sol ( #L15 ):
15: contract ERC20ProxyDelegator {

[N‑18] Contracts should have NatSpec @author tags

There are 2 instances:

  • ERC20MultiDelegate.sol ( #L15, #L25 ):
15: contract ERC20ProxyDelegator {

25: contract ERC20MultiDelegate is ERC1155, Ownable {

[N‑19] Contracts should have NatSpec @title tags

Some contract definitions have an incomplete NatSpec: add a @title notation to describe the contract to improve the code documentation.

There are 2 instances:

  • ERC20MultiDelegate.sol ( #L15, #L25 ):
15: contract ERC20ProxyDelegator {

25: contract ERC20MultiDelegate is ERC1155, Ownable {

[N‑20] Event declarations should have NatSpec descriptions

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:     );

[N‑21] NatSpec documentation for function is missing

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(

[N‑22] Functions missing NatSpec @param tag

There are 15 instances (click to show):
/// @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) {

[N‑23] Public variable declarations should have NatSpec descriptions

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;

[N‑24] Functions missing NatSpec @return tag

There are 3 instances:

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) {

[N‑25] Variables should be named in mixedCase style

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_uri

[N‑26] Non-assembly method available

There 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.chainid
  • assembly { 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)

[N‑27] Missing zero address check in functions with address parameters

Adding a zero address check for each address type parameter can prevent errors.

There are 13 instances (click to show):
/// @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) {

[N‑28] Named imports of parent contracts are missing

There are 2 instances:

  • ERC20MultiDelegate.sol ( #L25 ):
/// @audit ERC1155
/// @audit Ownable
25: contract ERC20MultiDelegate is ERC1155, Ownable {

[N‑29] Constants should be put on the left side of comparisons

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) {

[N‑30] Non-interface files should use fixed compiler versions

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;

[N‑31] High cyclomatic complexity

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):
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:     }

[N‑32] Consider bounding input array length

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 (

[N‑33] Use the latest Solidity version (0.8.19 for L2s)

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;

[N‑34] Use uint256/int256 instead of uint/int

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;

[N‑35] Functions with array parameters should have length checks in place

There are 3 instances:

/// @audit sources
/// @audit targets
/// @audit amounts
57:     function delegateMulti(
58:         uint256[] calldata sources,
59:         uint256[] calldata targets,
60:         uint256[] calldata amounts

[N‑36] Contract functions should use an interface

All 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 {

[N‑37] Control structures do not follow the Solidity Style Guide

Refer to the Solidity style guide - Control Structures.

There is 1 instance:

  • ERC20MultiDelegate.sol ( #L85 ):
85:         for (

[N‑38] Missing event for critical changes

Events should be emitted when critical changes are made to the contracts.

There is 1 instance:

151:     function setUri(string memory uri) external onlyOwner {
152:         _setURI(uri);
153:     }

[N‑39] Consider adding emergency-stop functionality

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 {

[N‑40] Consider adding a block/deny-list

Doing so will significantly increase centralization, but will help to prevent hackers from using stolen tokens

There are 2 instances:

  • ERC20MultiDelegate.sol ( #L15, #L25 ):
15: contract ERC20ProxyDelegator {

25: contract ERC20MultiDelegate is ERC1155, Ownable {

[N‑41] Enable IR-based code generation

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

[N‑42] Functions should have @notice tags

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(

[N‑43] Large or complicated code bases should implement invariant tests

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

[N‑44] Top-level declarations should be separated by at least two lines

There are 8 instances (click to show):
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(

[N‑45] Consider adding formal verification proofs

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:

[N‑46] Prevent re-setting a state variable with the same value

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);

Gas Optimizations

[G‑01] Constructors can be marked as payable to save deployment gas

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;

[G‑03] Using ++X/--Xinstead ofX++/X--` can save gas

It can save 5 gas for each execution / per iteration.

There is 1 instance:

  • ERC20MultiDelegate.sol ( #L88 ):
88:             transferIndex++

[G‑04] Use custom errors rather than revert()/require() strings to save gas

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(

[G‑06] internal functions only called once can be inlined to save gas

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):
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) {

[G‑07] Using this to access functions results in an external call, wasting gas

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)));

[G‑08] Functions that revert when called by normal users can be marked payable

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 {

[G‑09] Operator >=/<= costs less gas than operator >/<

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) {

[G‑10] require()/revert() strings longer than 32 bytes cost extra gas

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:         );

[G‑11] Increments can be unchecked to save gas

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:

85:         for (
86:             uint transferIndex = 0;
87:             transferIndex < Math.max(sourcesLength, targetsLength);
88:             transferIndex++

[G‑12] Using assembly to check for zero can save gas

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) {

[G‑13] Use assembly to write address/contract type storage values

Using assembly { sstore(state.slot, addr) instead of state = addr can save gas.

There is 1 instance:

  • ERC20MultiDelegate.sol ( #L48 ):
48:         token = _token;

[G‑14] Use != 0 or == 0 for unsigned integer comparison

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:

  • ERC20MultiDelegate.sol ( #L75, #L75 ):
/// @audit Replace with `!= 0`
75:             sourcesLength > 0 || targetsLength > 0,

/// @audit Replace with `!= 0`
75:             sourcesLength > 0 || targetsLength > 0,

[G‑15] Avoid contract existence checks by using low level calls

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)));

[G‑16] Avoid zero transfer to save gas

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);

[G‑17] Optimize names to save gas

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 {

[G‑18] Reduce gas usage by moving to Solidity 0.8.19 or later

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;

[G‑19] Newer versions of solidity are more gas efficient

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;

[G‑20] Avoid updating storage when the value hasn't changed

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);

[G‑21] The result of a function call should be cached rather than re-calling the function

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(

[G‑22] Use assembly to emit events

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);

[G‑23] Use assembly to compute hashes to save gas

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)

[G‑24] Use calldata instead of memory for immutable arguments

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 {

[G‑25] Consider activating via-ir for deploying

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:

Disputed Issues

[D‑01] abi.encodePacked() should be replaced with bytes.concat()

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:

/// @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:             )

[D‑02] Event names should use CamelCase

The instances below are already CamelCase (events are supposed to use CamelCase, not lowerCamelCase).

There are 2 instances:

  • ERC20MultiDelegate.sol ( #L32, #L33 ):
32:     event ProxyDeployed(address indexed delegate, address proxyAddress);

33:     event DelegationProcessed(

[D‑03] Cast to bytes or bytes32 for clearer semantic meaning

The rule is valid, but the following findings are invalid.

There are 2 instances:

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:             )

[D‑04] Return values of approve() not checked

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);

[D‑05] Event is missing indexed fields

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:

33:     event DelegationProcessed(
34:         address indexed from,
35:         address indexed to,
36:         uint256 amount
37:     );

[D‑06] Passing abi.encodePacked() with dynamic arguments to a hash can cause collisions

The cases below do not have multiple bytes/string arguments

There is 1 instance:

206:         bytes32 hash = keccak256(
207:             abi.encodePacked(
208:                 bytes1(0xff),
209:                 address(this),
210:                 uint256(0), // salt
211:                 keccak256(bytecode)
212:             )
213:         );

[D‑07] NatSpec: Contract declarations should have @notice tags

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:

  • ERC20MultiDelegate.sol ( #L15, #L25 ):
15: contract ERC20ProxyDelegator {

25: contract ERC20MultiDelegate is ERC1155, Ownable {

[D‑08] Not initializing local variables to zero saves gas

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;

[D‑09] safeMint should be used in place of mint

The following are not ERC721.mint() calls.

There is 1 instance:

  • ERC20MultiDelegate.sol ( #L114 ):
114:             _mintBatch(msg.sender, targets, amounts[:targetsLength], "");

[D‑10] Return values of transfer()/transferFrom() not checked

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);

[D‑11] SPDX identifier should be the in the first line of a solidity file

There is 1 instance:

  • ERC20MultiDelegate.sol ( #L1 ):
1: // SPDX-License-Identifier: MIT

[D‑12] Use != 0 or == 0 for unsigned integer comparison

Only 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) {

[D‑13] Avoid contract existence checks by using low level calls

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);

[D‑14] Array length is not checked before access its index

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:

57:     function delegateMulti(
58:         uint256[] calldata sources,
59:         uint256[] calldata targets,
60:         uint256[] calldata amounts
61:     ) external {

[D‑15] safeTransfer function does not check for contract existence

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:

  • ERC20MultiDelegate.sol ( #L16, #L44 ):
16:     constructor(ERC20Votes _token, address _delegate) {

44:     constructor(

[D‑16] Low level calls with solidity before 0.8.14 result in an optimiser bug

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);

[D‑18] Governance functions should be controlled by time locks

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 {

[D‑19] Assembly block creates dirty bits

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 {

[D‑20] Calculations should be memoized rather than re-calculating them

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

The rule is valid, but the following findings are invalid.

There is 1 instance:

/// @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:         );

[D‑22] Some tokens may revert when zero value transfers are made

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);

[D‑23] Use SafeCast to downcast safely

The rule is valid, but the following findings are invalid.

There is 1 instance:

  • ERC20MultiDelegate.sol ( #L210 ):
/// @audit uint256(0)
210:                 uint256(0), // salt

[D‑24] Unsafe use of ERC20 transfer()/transferFrom()

Some 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);

[D‑25] Use assembly to compute hashes to save gas

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:

206:         bytes32 hash = keccak256(
207:             abi.encodePacked(
208:                 bytes1(0xff),
209:                 address(this),
210:                 uint256(0), // salt
211:                 keccak256(bytecode)
212:             )
213:         );

[D‑26] Contracts should have full test coverage

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

[D‑27] Use descriptive constant instead of 0 as a parameter

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:

207:             abi.encodePacked(
208:                 bytes1(0xff),
209:                 address(this),
210:                 uint256(0), // salt
211:                 keccak256(bytecode)
212:             )

[D‑28] Some tokens may revert when large transfers are made

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);

[D‑29] Contracts are vulnerable to rebasing accounting-related issues

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:

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 {
@thebrittfactor
Copy link

For transparency, the sponsor responded in regards to M-01:

I don't view it as valid; URIs are part of 1155, and even if we made the URI immutable, the content at the URL could still change.
It's also explicitly part of the design of the contract, not a vulnerability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment