Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Gas Optimizations #220

Open
code423n4 opened this issue Jul 19, 2022 · 1 comment
Open

Gas Optimizations #220

code423n4 opened this issue Jul 19, 2022 · 1 comment
Labels
bug Something isn't working G (Gas Optimization) old-submission-method

Comments

@code423n4
Copy link
Contributor

Summary

Gas Optimizations

Issue Instances
[G‑01] Calculation re-calculated in same function 2
[G‑02] Using calldata instead of memory for read-only arguments in external functions saves gas 4
[G‑03] Avoid contract existence checks by using solidity version 0.8.10 or later 3
[G‑04] internal functions only called once can be inlined to save gas 14
[G‑05] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement 2
[G‑06] <array>.length should not be looked up in every loop of a for-loop 4
[G‑07] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops 7
[G‑08] require()/revert() strings longer than 32 bytes cost extra gas 26
[G‑09] Optimize names to save gas 17
[G‑10] Using bools for storage incurs overhead 2
[G‑11] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) 7
[G‑12] Splitting require() statements that use && saves gas 3
[G‑13] Using private rather than public for constants, saves gas 3
[G‑14] require() or revert() statements that check input arguments should be at the top of the function 4
[G‑15] Use custom errors rather than revert()/require() strings to save gas 26
[G‑16] Functions guaranteed to revert when called by normal users can be marked payable 14

Total: 138 instances over 16 issues

Gas Optimizations

[G‑01] Calculation re-calculated in same function

The calculation is repeated multiple times in the function. Instead, the value should be cache and re-used

There are 2 instances of this issue:

File: contracts/ethregistrar/ETHRegistrarController.sol

182:         if (msg.value > (price.base + price.premium)) {

184:                 msg.value - (price.base + price.premium)

https://github.com/code-423n4/2022-07-ens/blob/4dfb0e32f586bff3db486349523a93480e3ddfba/contracts/ethregistrar/ETHRegistrarController.sol#L182

[G‑02] Using calldata instead of memory for read-only arguments in external functions saves gas

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it's still valid for implementation contracs to use calldata arguments instead.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one

Note that I've also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved

There are 4 instances of this issue:

File: contracts/dnssec-oracle/DNSSECImpl.sol

/// @audit _anchors
46:       constructor(bytes memory _anchors) {

/// @audit input
80:       function verifyRRSet(RRSetWithSignature[] memory input) external virtual view override returns(bytes memory) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/DNSSECImpl.sol#L46

File: contracts/dnssec-oracle/DNSSEC.sol

/// @audit input
17:       function verifyRRSet(RRSetWithSignature[] memory input) external virtual view returns(bytes memory);

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/DNSSEC.sol#L17

File: contracts/wrapper/NameWrapper.sol

/// @audit label
539       function setSubnodeRecord(
540           bytes32 parentNode,
541           string memory label,
542           address newOwner,
543           address resolver,
544           uint64 ttl,
545           uint32 fuses,
546           uint64 expiry
547       )
548           public
549           onlyTokenOwner(parentNode)
550:          canCallSetSubnodeOwner(parentNode, keccak256(bytes(label)))

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L539-L550

[G‑03] Avoid contract existence checks by using solidity version 0.8.10 or later

Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (700 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value

There are 3 instances of this issue:

File: contracts/registry/ReverseRegistrar.sol

/// @audit owner()
182:          try Ownable(addr).owner() returns (address owner) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/registry/ReverseRegistrar.sol#L182

File: contracts/wrapper/ERC1155Fuse.sol

/// @audit onERC1155Received()
311:                  IERC1155Receiver(to).onERC1155Received(

/// @audit onERC1155BatchReceived()
342:                  IERC1155Receiver(to).onERC1155BatchReceived(

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/ERC1155Fuse.sol#L311

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

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

There are 14 instances of this issue:

File: contracts/dnssec-oracle/DNSSECImpl.sol

110:      function validateSignedSet(RRSetWithSignature memory input, bytes memory proof, uint256 now) internal view returns(RRUtils.SignedSet memory rrset) {

147:      function validateRRs(RRUtils.SignedSet memory rrset, uint16 typecovered) internal pure returns (bytes memory name) {

183:      function verifySignature(bytes memory name, RRUtils.SignedSet memory rrset, RRSetWithSignature memory data, bytes memory proof) internal view {

209:      function verifyWithKnownKey(RRUtils.SignedSet memory rrset, RRSetWithSignature memory data, RRUtils.RRIterator memory proof) internal view {

273:      function verifyWithDS(RRUtils.SignedSet memory rrset, RRSetWithSignature memory data, RRUtils.RRIterator memory proof) internal view {

299       function verifyKeyWithDS(bytes memory keyname, RRUtils.RRIterator memory dsrrs, RRUtils.DNSKEY memory dnskey, bytes memory keyrdata)
300:          internal view returns (bool)

335:      function verifyDSHash(uint8 digesttype, bytes memory data, bytes memory digest) internal view returns (bool) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/DNSSECImpl.sol#L110

File: contracts/ethregistrar/ETHRegistrarController.sol

226       function _consumeCommitment(
227           string memory name,
228           uint256 duration,
229:          bytes32 commitment

249       function _setRecords(
250           address resolver,
251           bytes32 label,
252:          bytes[] calldata data

270       function _setReverseRecord(
271           string memory name,
272           address resolver,
273:          address owner

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L226-L229

File: contracts/registry/ReverseRegistrar.sol

181:      function ownsContract(address addr) internal view returns (bool) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/registry/ReverseRegistrar.sol#L181

File: contracts/wrapper/NameWrapper.sol

741       function _addLabel(string memory label, bytes memory name)
742           internal
743           pure
744:          returns (bytes memory ret)

755       function _mint(
756           bytes32 node,
757           address wrappedOwner,
758           uint32 fuses,
759:          uint64 expiry

846       function _getETH2LDDataAndNormaliseExpiry(
847           bytes32 node,
848           bytes32 labelhash,
849           uint64 expiry
850       )
851           internal
852           view
853           returns (
854               address owner,
855               uint32 fuses,
856:              uint64

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L741-L744

[G‑05] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement

require(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }

There are 2 instances of this issue:

File: contracts/ethregistrar/ETHRegistrarController.sol

/// @audit require() on line 197
/// @audit if-condition on line 203
204:              payable(msg.sender).transfer(msg.value - price.base);

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L204

[G‑06] <array>.length should not be looked up in every loop of a for-loop

The overheads outlined below are PER LOOP, excluding the first loop

  • storage arrays incur a Gwarmaccess (100 gas)
  • memory arrays use MLOAD (3 gas)
  • calldata arrays use CALLDATALOAD (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

There are 4 instances of this issue:

File: contracts/dnssec-oracle/DNSSECImpl.sol

93:           for(uint i = 0; i < input.length; i++) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/DNSSECImpl.sol#L93

File: contracts/ethregistrar/ETHRegistrarController.sol

256:          for (uint256 i = 0; i < data.length; i++) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L256

File: contracts/wrapper/ERC1155Fuse.sol

92:           for (uint256 i = 0; i < accounts.length; ++i) {

205:          for (uint256 i = 0; i < ids.length; ++i) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/ERC1155Fuse.sol#L92

[G‑07] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

There are 7 instances of this issue:

File: contracts/dnssec-oracle/BytesUtils.sol

266:          for(uint i = 0; i < len; i++) {

313:          for(uint256 idx = off; idx < off + len; idx++) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/BytesUtils.sol#L266

File: contracts/dnssec-oracle/DNSSECImpl.sol

93:           for(uint i = 0; i < input.length; i++) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/DNSSECImpl.sol#L93

File: contracts/ethregistrar/ETHRegistrarController.sol

256:          for (uint256 i = 0; i < data.length; i++) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L256

File: contracts/ethregistrar/StringUtils.sol

14:           for(len = 0; i < bytelength; len++) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/StringUtils.sol#L14

File: contracts/wrapper/ERC1155Fuse.sol

92:           for (uint256 i = 0; i < accounts.length; ++i) {

205:          for (uint256 i = 0; i < ids.length; ++i) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/ERC1155Fuse.sol#L92

[G‑08] 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 26 instances of this issue:

File: contracts/ethregistrar/ETHRegistrarController.sol

99                require(
100                   resolver != address(0),
101                   "ETHRegistrarController: resolver is required when data is supplied"
102:              );

137           require(
138               msg.value >= (price.base + price.premium),
139               "ETHRegistrarController: Not enough ether provided"
140:          );

196           require(
197               msg.value >= price.base,
198               "ETHController: Not enough Ether provided for renewal"
199:          );

232           require(
233               commitments[commitment] + minCommitmentAge <= block.timestamp,
234               "ETHRegistrarController: Commitment is not valid"
235:          );

238           require(
239               commitments[commitment] + maxCommitmentAge > block.timestamp,
240               "ETHRegistrarController: Commitment has expired"
241:          );

242:          require(available(name), "ETHRegistrarController: Name is unavailable");

259               require(
260                   txNamehash == nodehash,
261                   "ETHRegistrarController: Namehash on record do not match the name being registered"
262:              );

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L99-L102

File: contracts/registry/ReverseRegistrar.sol

41            require(
42                addr == msg.sender ||
43                    controllers[msg.sender] ||
44                    ens.isApprovedForAll(addr, msg.sender) ||
45                    ownsContract(addr),
46                "ReverseRegistrar: Caller is not a controller or authorised by address or the address itself"
47:           );

52            require(
53                address(resolver) != address(0),
54                "ReverseRegistrar: Resolver address must not be 0"
55:           );

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/registry/ReverseRegistrar.sol#L41-L47

File: contracts/wrapper/Controllable.sol

17:           require(controllers[msg.sender], "Controllable: Caller is not a controller");

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/Controllable.sol#L17

File: contracts/wrapper/ERC1155Fuse.sol

60            require(
61                account != address(0),
62                "ERC1155: balance query for the zero address"
63:           );

85            require(
86                accounts.length == ids.length,
87                "ERC1155: accounts and ids length mismatch"
88:           );

107           require(
108               msg.sender != operator,
109               "ERC1155: setting approval status for self"
110:          );

176:          require(to != address(0), "ERC1155: transfer to the zero address");

177           require(
178               from == msg.sender || isApprovedForAll(from, msg.sender),
179               "ERC1155: caller is not owner nor approved"
180:          );

195           require(
196               ids.length == amounts.length,
197               "ERC1155: ids and amounts length mismatch"
198:          );

199:          require(to != address(0), "ERC1155: transfer to the zero address");

200           require(
201               from == msg.sender || isApprovedForAll(from, msg.sender),
202               "ERC1155: transfer caller is not owner nor approved"
203:          );

215               require(
216                   amount == 1 && oldOwner == from,
217                   "ERC1155: insufficient balance for transfer"
218:              );

249:          require(newOwner != address(0), "ERC1155: mint to the zero address");

250           require(
251               newOwner != address(this),
252               "ERC1155: newOwner cannot be the NameWrapper contract"
253:          );

290           require(
291               amount == 1 && oldOwner == from,
292               "ERC1155: insufficient balance for transfer"
293:          );

322:                      revert("ERC1155: ERC1155Receiver rejected tokens");

327:                  revert("ERC1155: transfer to non ERC1155Receiver implementer");

354:                      revert("ERC1155: ERC1155Receiver rejected tokens");

359:                  revert("ERC1155: transfer to non ERC1155Receiver implementer");

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/ERC1155Fuse.sol#L60-L63

[G‑09] Optimize names to save gas

public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. 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 are 17 instances of this issue:

File: contracts/dnssec-oracle/algorithms/Algorithm.sol

/// @audit verify()
6:    interface Algorithm {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/algorithms/Algorithm.sol#L6

File: contracts/dnssec-oracle/digests/Digest.sol

/// @audit verify()
6:    interface Digest {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/digests/Digest.sol#L6

File: contracts/dnssec-oracle/DNSSECImpl.sol

/// @audit setAlgorithm(), setDigest()
16:   contract DNSSECImpl is DNSSEC, Owned {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/DNSSECImpl.sol#L16

File: contracts/dnssec-oracle/DNSSEC.sol

/// @audit verifyRRSet(), verifyRRSet()
5:    abstract contract DNSSEC {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/DNSSEC.sol#L5

File: contracts/dnssec-oracle/Owned.sol

/// @audit setOwner()
6:    contract Owned {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/Owned.sol#L6

File: contracts/ethregistrar/ETHRegistrarController.sol

/// @audit valid(), withdraw()
17:   contract ETHRegistrarController is Ownable, IETHRegistrarController {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L17

File: contracts/ethregistrar/IBaseRegistrar.sol

/// @audit addController(), removeController(), setResolver(), nameExpires(), available(), register(), renew(), reclaim()
5:    interface IBaseRegistrar is IERC721 {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/IBaseRegistrar.sol#L5

File: contracts/ethregistrar/IETHRegistrarController.sol

/// @audit rentPrice(), available(), makeCommitment(), commit(), register(), renew()
5:    interface IETHRegistrarController {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/IETHRegistrarController.sol#L5

File: contracts/registry/ENS.sol

/// @audit setRecord(), setSubnodeRecord(), setSubnodeOwner(), setResolver(), setOwner(), setTTL(), owner(), resolver(), ttl(), recordExists()
3:    interface ENS {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/registry/ENS.sol#L3

File: contracts/registry/IReverseRegistrar.sol

/// @audit setDefaultResolver(), claim(), claimForAddr(), claimWithResolver(), setName(), setNameForAddr(), node()
3:    interface IReverseRegistrar {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/registry/IReverseRegistrar.sol#L3

File: contracts/registry/ReverseRegistrar.sol

/// @audit setName()
8:    abstract contract NameResolver {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/registry/ReverseRegistrar.sol#L8

File: contracts/resolvers/Resolver.sol

/// @audit setABI(), setAddr(), setAddr(), setContenthash(), setDnsrr(), setName(), setPubkey(), setText(), setInterface(), multicall(), content(), multihash(), setContent(), setMultihash()
20:   interface Resolver is

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/resolvers/Resolver.sol#L20

File: contracts/wrapper/Controllable.sol

/// @audit setController()
6:    contract Controllable is Ownable {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/Controllable.sol#L6

File: contracts/wrapper/ERC1155Fuse.sol

/// @audit getData()
14:   abstract contract ERC1155Fuse is ERC165, IERC1155, IERC1155MetadataURI {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/ERC1155Fuse.sol#L14

File: contracts/wrapper/INameWrapper.sol

/// @audit ens(), registrar(), metadataService(), names(), wrap(), wrapETH2LD(), registerAndWrapETH2LD(), renew(), unwrap(), unwrapETH2LD(), setFuses(), setChildFuses(), setSubnodeRecord(), setRecord(), setSubnodeOwner(), isTokenOwnerOrApproved(), setResolver(), setTTL(), getFuses(), allFusesBurned()
18:   interface INameWrapper is IERC1155 {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/INameWrapper.sol#L18

File: contracts/wrapper/INameWrapperUpgrade.sol

/// @audit setSubnodeRecord(), wrapETH2LD()
4:    interface INameWrapperUpgrade {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/INameWrapperUpgrade.sol#L4

File: contracts/wrapper/NameWrapper.sol

/// @audit setMetadataService(), setUpgradeContract(), setFuses(), upgradeETH2LD(), upgrade(), setChildFuses(), setSubnodeOwner(), setSubnodeRecord()
27:   contract NameWrapper is

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L27

[G‑10] Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past

There are 2 instances of this issue:

File: contracts/wrapper/Controllable.sol

7:        mapping(address=>bool) public controllers;

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/Controllable.sol#L7

File: contracts/wrapper/ERC1155Fuse.sol

19:       mapping(address => mapping(address => bool)) private _operatorApprovals;

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/ERC1155Fuse.sol#L19

[G‑11] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 5 gas per loop

There are 7 instances of this issue:

File: contracts/dnssec-oracle/BytesUtils.sol

266:          for(uint i = 0; i < len; i++) {

313:          for(uint256 idx = off; idx < off + len; idx++) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/BytesUtils.sol#L266

File: contracts/dnssec-oracle/DNSSECImpl.sol

93:           for(uint i = 0; i < input.length; i++) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/DNSSECImpl.sol#L93

File: contracts/dnssec-oracle/RRUtils.sol

235:              counts--;

241:              othercounts--;

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/RRUtils.sol#L235

File: contracts/ethregistrar/ETHRegistrarController.sol

256:          for (uint256 i = 0; i < data.length; i++) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L256

File: contracts/ethregistrar/StringUtils.sol

14:           for(len = 0; i < bytelength; len++) {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/StringUtils.sol#L14

[G‑12] Splitting require() statements that use && saves gas

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper by 3 gas

There are 3 instances of this issue:

File: contracts/dnssec-oracle/BytesUtils.sol

268:              require(char >= 0x30 && char <= 0x7A);

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/BytesUtils.sol#L268

File: contracts/wrapper/ERC1155Fuse.sol

215               require(
216                   amount == 1 && oldOwner == from,
217                   "ERC1155: insufficient balance for transfer"
218:              );

290           require(
291               amount == 1 && oldOwner == from,
292               "ERC1155: insufficient balance for transfer"
293:          );

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/ERC1155Fuse.sol#L215-L218

[G‑13] Using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

There are 3 instances of this issue:

File: contracts/ethregistrar/ETHRegistrarController.sol

21:       uint256 public constant MIN_REGISTRATION_DURATION = 28 days;

27:       uint256 public immutable minCommitmentAge;

28:       uint256 public immutable maxCommitmentAge;

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L21

[G‑14] require() or revert() statements that check input arguments should be at the top of the function

Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas*) in a function that may ultimately revert in the unhappy case.

There are 4 instances of this issue:

File: contracts/ethregistrar/ETHRegistrarController.sol

/// @audit expensive op on line 244
246:          require(duration >= MIN_REGISTRATION_DURATION);

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L246

File: contracts/wrapper/ERC1155Fuse.sol

/// @audit expensive op on line 196
199:          require(to != address(0), "ERC1155: transfer to the zero address");

/// @audit expensive op on line 248
249:          require(newOwner != address(0), "ERC1155: mint to the zero address");

/// @audit expensive op on line 248
250           require(
251               newOwner != address(this),
252               "ERC1155: newOwner cannot be the NameWrapper contract"
253:          );

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/ERC1155Fuse.sol#L199

[G‑15] 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 26 instances of this issue:

File: contracts/dnssec-oracle/RRUtils.sol

307:              require(data.length <= 8192, "Long keys not permitted");

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/RRUtils.sol#L307

File: contracts/ethregistrar/ETHRegistrarController.sol

99                require(
100                   resolver != address(0),
101                   "ETHRegistrarController: resolver is required when data is supplied"
102:              );

137           require(
138               msg.value >= (price.base + price.premium),
139               "ETHRegistrarController: Not enough ether provided"
140:          );

196           require(
197               msg.value >= price.base,
198               "ETHController: Not enough Ether provided for renewal"
199:          );

232           require(
233               commitments[commitment] + minCommitmentAge <= block.timestamp,
234               "ETHRegistrarController: Commitment is not valid"
235:          );

238           require(
239               commitments[commitment] + maxCommitmentAge > block.timestamp,
240               "ETHRegistrarController: Commitment has expired"
241:          );

242:          require(available(name), "ETHRegistrarController: Name is unavailable");

259               require(
260                   txNamehash == nodehash,
261                   "ETHRegistrarController: Namehash on record do not match the name being registered"
262:              );

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L99-L102

File: contracts/registry/ReverseRegistrar.sol

41            require(
42                addr == msg.sender ||
43                    controllers[msg.sender] ||
44                    ens.isApprovedForAll(addr, msg.sender) ||
45                    ownsContract(addr),
46                "ReverseRegistrar: Caller is not a controller or authorised by address or the address itself"
47:           );

52            require(
53                address(resolver) != address(0),
54                "ReverseRegistrar: Resolver address must not be 0"
55:           );

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/registry/ReverseRegistrar.sol#L41-L47

File: contracts/wrapper/BytesUtil.sol

28:               require(offset == self.length - 1, "namehash: Junk at end of name");

42:           require(idx < self.length, "readLabel: Index out of bounds");

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/BytesUtil.sol#L28

File: contracts/wrapper/Controllable.sol

17:           require(controllers[msg.sender], "Controllable: Caller is not a controller");

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/Controllable.sol#L17

File: contracts/wrapper/ERC1155Fuse.sol

60            require(
61                account != address(0),
62                "ERC1155: balance query for the zero address"
63:           );

85            require(
86                accounts.length == ids.length,
87                "ERC1155: accounts and ids length mismatch"
88:           );

107           require(
108               msg.sender != operator,
109               "ERC1155: setting approval status for self"
110:          );

176:          require(to != address(0), "ERC1155: transfer to the zero address");

177           require(
178               from == msg.sender || isApprovedForAll(from, msg.sender),
179               "ERC1155: caller is not owner nor approved"
180:          );

195           require(
196               ids.length == amounts.length,
197               "ERC1155: ids and amounts length mismatch"
198:          );

199:          require(to != address(0), "ERC1155: transfer to the zero address");

200           require(
201               from == msg.sender || isApprovedForAll(from, msg.sender),
202               "ERC1155: transfer caller is not owner nor approved"
203:          );

215               require(
216                   amount == 1 && oldOwner == from,
217                   "ERC1155: insufficient balance for transfer"
218:              );

248:          require(owner == address(0), "ERC1155: mint of existing token");

249:          require(newOwner != address(0), "ERC1155: mint to the zero address");

250           require(
251               newOwner != address(this),
252               "ERC1155: newOwner cannot be the NameWrapper contract"
253:          );

290           require(
291               amount == 1 && oldOwner == from,
292               "ERC1155: insufficient balance for transfer"
293:          );

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/ERC1155Fuse.sol#L60-L63

[G‑16] Functions guaranteed to 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 costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 14 instances of this issue:

File: contracts/registry/ReverseRegistrar.sol

51:       function setDefaultResolver(address resolver) public override onlyOwner {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/registry/ReverseRegistrar.sol#L51

File: contracts/wrapper/Controllable.sol

11:       function setController(address controller, bool active) onlyOwner() public {

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/Controllable.sol#L11

File: contracts/wrapper/NameWrapper.sol

105       function setMetadataService(IMetadataService _newMetadataService)
106           public
107:          onlyOwner

128       function setUpgradeContract(INameWrapperUpgrade _upgradeAddress)
129           public
130:          onlyOwner

251       function registerAndWrapETH2LD(
252           string calldata label,
253           address wrappedOwner,
254           uint256 duration,
255           address resolver,
256           uint32 fuses,
257           uint64 expiry
258:      ) external override onlyController returns (uint256 registrarExpiry) {

271       function renew(
272           uint256 tokenId,
273           uint256 duration,
274           uint64 expiry
275:      ) external override onlyController returns (uint256 expires) {

335       function unwrapETH2LD(
336           bytes32 labelhash,
337           address newRegistrant,
338           address newController
339:      ) public override onlyTokenOwner(_makeNode(ETH_NODE, labelhash)) {

356       function unwrap(
357           bytes32 parentNode,
358           bytes32 labelhash,
359           address newController
360:      ) public override onlyTokenOwner(_makeNode(parentNode, labelhash)) {

373       function setFuses(bytes32 node, uint32 fuses)
374           public
375           onlyTokenOwner(node)
376           operationAllowed(node, CANNOT_BURN_FUSES)
377:          returns (uint32)

504       function setSubnodeOwner(
505           bytes32 parentNode,
506           string calldata label,
507           address newOwner,
508           uint32 fuses,
509           uint64 expiry
510       )
511           public
512           onlyTokenOwner(parentNode)
513           canCallSetSubnodeOwner(parentNode, keccak256(bytes(label)))
514:          returns (bytes32 node)

539       function setSubnodeRecord(
540           bytes32 parentNode,
541           string memory label,
542           address newOwner,
543           address resolver,
544           uint64 ttl,
545           uint32 fuses,
546           uint64 expiry
547       )
548           public
549           onlyTokenOwner(parentNode)
550:          canCallSetSubnodeOwner(parentNode, keccak256(bytes(label)))

584       function setRecord(
585           bytes32 node,
586           address owner,
587           address resolver,
588           uint64 ttl
589       )
590           public
591           override
592           onlyTokenOwner(node)
593           operationAllowed(
594               node,
595               CANNOT_TRANSFER | CANNOT_SET_RESOLVER | CANNOT_SET_TTL
596:          )

609       function setResolver(bytes32 node, address resolver)
610           public
611           override
612           onlyTokenOwner(node)
613:          operationAllowed(node, CANNOT_SET_RESOLVER)

624       function setTTL(bytes32 node, uint64 ttl)
625           public
626           override
627           onlyTokenOwner(node)
628:          operationAllowed(node, CANNOT_SET_TTL)

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L105-L107
code423n4 added a commit that referenced this issue Jul 19, 2022
@code423n4
IllIllI issue #220
02b4448
@jefflau
Copy link
Collaborator

jefflau commented Aug 1, 2022

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

Some of these internal functions are dealing with the stack limit in solidity.

High quality submission, well documented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working G (Gas Optimization) old-submission-method
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jefflau @code423n4