Platform: Code4rena
Start Date: 12/07/2022
Pot Size: $75,000 USDC
Total HM: 16
Participants: 100
Period: 7 days
Judge: LSDan
Total Solo HM: 7
Id: 145
League: ETH
Rank: 16/100
Findings: 3
Award: $936.66
š Selected for report: 1
š Solo Findings: 0
š Selected for report: rajatbeladiya
Also found by: 0x29A, 0xNineDec, Amithuddar, Aussie_Battlers, Ch_301, Dravee, GimelSec, IllIllI, Jujic, Limbooo, RedOneN, Ruhum, TomJ, _Adam, __141345__, alan724, asutorufos, berndartmueller, c3phas, cccz, cryptphi, durianSausage, fatherOfBlocks, hake, hyh, pashov, scaraven, zzzitron
5.45 USDC - $5.45
Judge has assessed an item in Issue #221 as Medium risk. The relevant finding follows:
payable.transfer()/payable.send()The use of payable.transfer() is heavily frowned upon because it can lead to the locking of funds. The transfer() call requires that the recipient is either an EOA account, or is a contract that has a payable callback. For the contract case, the transfer() call only provides 2300 gas for the contract to complete its operations. This means the following cases can cause the transfer to fail:
payable callbackpayable callback spends more than 2300 gas (which is only enough to emit something)Address.sendValue() insteadThere are 3 instances of this issue:
File: contracts/ethregistrar/ETHRegistrarController.sol 183 payable(msg.sender).transfer( 184 msg.value - (price.base + price.premium) 185: ); 204: payable(msg.sender).transfer(msg.value - price.base); 211: payable(owner()).transfer(address(this).balance);
#0 - dmvt
2022-10-14T09:13:02Z
Duplicate of #133
š Selected for report: IllIllI
Also found by: 0x1f8b, 0x29A, 0xDjango, 0xNazgul, 0xNineDec, 0xf15ers, 8olidity, Aussie_Battlers, Bnke0x0, Ch_301, Critical, Deivitto, Dravee, ElKu, Funen, GimelSec, JC, JohnSmith, Lambda, MiloTruck, PwnedNoMore, ReyAdmirado, Rohan16, Rolezn, Ruhum, RustyRabbit, Sm4rty, TomJ, Waze, _Adam, __141345__, alan724, asutorufos, benbaessler, berndartmueller, bin2chen, brgltd, bulej93, c3phas, cRat1st0s, cryptonue, cryptphi, csanuragjain, delfin454000, dxdv, exd0tpy, fatherOfBlocks, gogo, hake, hyh, joestakey, kyteg, lcfr_eth, minhtrng, p_crypt0, pashov, pedr02b2, philogy, rajatbeladiya, rbserver, rishabh, robee, rokinot, sach1r0, sashik_eth, seyni, simon135, svskaushik, zuhaibmohd, zzzitron
772.0432 USDC - $772.04
| Issue | Instances | |
|---|---|---|
| [Lā01] | Don't use payable.transfer()/payable.send() | 3 |
| [Lā02] | require() should be used instead of assert() | 2 |
| [Lā03] | now is deprecated | 5 |
| [Lā04] | Missing checks for address(0x0) when assigning values to address state variables | 1 |
| [Lā05] | Open TODOs | 1 |
Total: 12 instances over 5 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | Name validation is not strictly valid | 1 |
| [Nā02] | Adding a return statement when the function defines a named return variable, is redundant | 1 |
| [Nā03] | require()/revert() statements should have descriptive reason strings | 17 |
| [Nā04] | public functions not called by the contract should be declared external instead | 13 |
| [Nā05] | constants should be defined rather than using magic numbers | 150 |
| [Nā06] | Redundant cast | 1 |
| [Nā07] | Missing event and or timelock for critical parameter change | 3 |
| [Nā08] | File is missing version pragma | 1 |
| [Nā09] | Use a more recent version of solidity | 4 |
| [Nā10] | Use a more recent version of solidity | 3 |
| [Nā11] | pragma experimental ABIEncoderV2 is deprecated | 2 |
| [Nā12] | Constant redefined elsewhere | 1 |
| [Nā13] | Inconsistent spacing in comments | 2 |
| [Nā14] | Lines are too long | 8 |
| [Nā15] | Inconsistent method of specifying a floating pragma | 10 |
| [Nā16] | Non-library/interface files should use fixed compiler versions, not floating ones | 4 |
| [Nā17] | Typos | 5 |
| [Nā18] | File does not contain an SPDX Identifier | 14 |
| [Nā19] | File is missing NatSpec | 10 |
| [Nā20] | NatSpec is incomplete | 13 |
| [Nā21] | Event is missing indexed fields | 18 |
| [Nā22] | Not using the named return variables anywhere in the function is confusing | 4 |
| [Nā23] | Duplicated require()/revert() checks should be refactored to a modifier or function | 6 |
Total: 291 instances over 23 issues
payable.transfer()/payable.send()The use of payable.transfer() is heavily frowned upon because it can lead to the locking of funds. The transfer() call requires that the recipient is either an EOA account, or is a contract that has a payable callback. For the contract case, the transfer() call only provides 2300 gas for the contract to complete its operations. This means the following cases can cause the transfer to fail:
payable callbackpayable callback spends more than 2300 gas (which is only enough to emit something)Address.sendValue() insteadThere are 3 instances of this issue:
File: contracts/ethregistrar/ETHRegistrarController.sol 183 payable(msg.sender).transfer( 184 msg.value - (price.base + price.premium) 185: ); 204: payable(msg.sender).transfer(msg.value - price.base); 211: payable(owner()).transfer(address(this).balance);
require() should be used instead of assert()Prior to solidity version 0.8.0, hitting an assert consumes the remainder of the transaction's available gas rather than returning it, as require()/revert() do. assert() should be avoided even past solidity version 0.8.0 as its documentation states that "The assert function creates an error of type Panic(uint256). ... 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 are 2 instances of this issue:
File: contracts/dnssec-oracle/RRUtils.sol 22: assert(idx < self.length); 52: assert(offset < self.length);
now is deprecatedUse block.timestamp instead. This may not cause problems now, but if a bug is discovered in this release, and you're forced to change to another version, now may be unavailable, causing unnecessary delays in porting and testing the new contracts.
There are 5 instances of this issue:
File: contracts/dnssec-oracle/DNSSECImpl.sol 94: RRUtils.SignedSet memory rrset = validateSignedSet(input[i], proof, now); 126: if(!RRUtils.serialNumberGte(rrset.expiration, uint32(now))) { 127: revert SignatureExpired(rrset.expiration, uint32(now)); 132: if(!RRUtils.serialNumberGte(uint32(now), rrset.inception)) { 133: revert SignatureNotValidYet(rrset.inception, uint32(now));
address(0x0) when assigning values to address state variablesThere is 1 instance of this issue:
File: contracts/dnssec-oracle/Owned.sol 19: owner = newOwner;
Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment
There is 1 instance of this issue:
File: contracts/dnssec-oracle/DNSSECImpl.sol 238: // TODO: Check key isn't expired, unless updating key itself
While the documentation does in fact say that there are other validations necessary to be compatible with the legacy DNS system, it would be better to have the following function signature instead function valid(string calldata name, bool isEnforceLegacyRules) public pure returns (bool), so it's clear what the caller is validating
There is 1 instance of this issue:
File: contracts/ethregistrar/ETHRegistrarController.sol 77 function valid(string memory name) public pure returns (bool) { 78 return name.strlen() >= 3; 79: }
return statement when the function defines a named return variable, is redundantThere is 1 instance of this issue:
File: contracts/dnssec-oracle/DNSSECImpl.sol 139: return rrset;
require()/revert() statements should have descriptive reason stringsThere are 17 instances of this issue:
File: contracts/dnssec-oracle/BytesUtils.sol 12: require(offset + len <= self.length); 146: require(idx + 2 <= self.length); 159: require(idx + 4 <= self.length); 172: require(idx + 32 <= self.length); 185: require(idx + 20 <= self.length); 199: require(len <= 32); 200: require(idx + len <= self.length); 235: require(offset + len <= self.length); 262: require(len <= 52); 268: require(char >= 0x30 && char <= 0x7A); 270: require(decoded <= 0x20); 298: revert();
File: contracts/dnssec-oracle/Owned.sol 10: require(msg.sender == owner);
File: contracts/ethregistrar/ETHRegistrarController.sol 57: require(_maxCommitmentAge > _minCommitmentAge); 121: require(commitments[commitment] + maxCommitmentAge < block.timestamp); 246: require(duration >= MIN_REGISTRATION_DURATION);
File: contracts/wrapper/BytesUtil.sol 13: require(offset + len <= self.length);
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
There are 13 instances of this issue:
File: contracts/dnssec-oracle/DNSSECImpl.sol 58: function setAlgorithm(uint8 id, Algorithm algo) public owner_only { 69: function setDigest(uint8 id, Digest digest) public owner_only {
File: contracts/dnssec-oracle/Owned.sol 18: function setOwner(address newOwner) public owner_only {
File: contracts/ethregistrar/ETHRegistrarController.sol 210 function withdraw() public { 211: payable(owner()).transfer(address(this).balance);
File: contracts/wrapper/Controllable.sol 11: function setController(address controller, bool active) onlyOwner() public {
File: contracts/wrapper/NameWrapper.sol 105 function setMetadataService(IMetadataService _newMetadataService) 106 public 107: onlyOwner 128 function setUpgradeContract(INameWrapperUpgrade _upgradeAddress) 129 public 130: onlyOwner 373 function setFuses(bytes32 node, uint32 fuses) 374 public 375 onlyTokenOwner(node) 376 operationAllowed(node, CANNOT_BURN_FUSES) 377: returns (uint32) 401 function upgradeETH2LD( 402 string calldata label, 403 address wrappedOwner, 404: address resolver 429 function upgrade( 430 bytes32 parentNode, 431 string calldata label, 432 address wrappedOwner, 433: address resolver 456 function setChildFuses( 457 bytes32 parentNode, 458 bytes32 labelhash, 459 uint32 fuses, 460: uint64 expiry 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)))
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 150 instances of this issue:
File: contracts/dnssec-oracle/BytesUtils.sol /// @audit 32 56: for (uint idx = 0; idx < shortest; idx += 32) { /// @audit 32 66: if (shortest > 32) { /// @audit 8 /// @audit 32 69: mask = ~(2 ** (8 * (32 - shortest + idx)) - 1); /// @audit 32 75: selfptr += 32; /// @audit 32 76: otherptr += 32; /// @audit 0xFFFF 148: ret := and(mload(add(add(self, 2), idx)), 0xFFFF) /// @audit 4 159: require(idx + 4 <= self.length); /// @audit 0xFFFFFFFF 161: ret := and(mload(add(add(self, 4), idx)), 0xFFFFFFFF) /// @audit 32 172: require(idx + 32 <= self.length); /// @audit 20 185: require(idx + 20 <= self.length); /// @audit 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF000000000000000000000000 187: ret := and(mload(add(add(self, 32), idx)), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF000000000000000000000000) /// @audit 32 199: require(len <= 32); /// @audit 32 /// @audit 32 209: for (; len >= 32; len -= 32) { /// @audit 32 213: dest += 32; /// @audit 32 214: src += 32; /// @audit 256 /// @audit 32 219: uint mask = (256 ** (32 - len)) - 1; /// @audit 52 262: require(len <= 52); /// @audit 0x30 /// @audit 0x7A 268: require(char >= 0x30 && char <= 0x7A); /// @audit 0x30 269: decoded = uint8(base32HexTable[uint(uint8(char)) - 0x30]); /// @audit 0x20 270: require(decoded <= 0x20); /// @audit 5 274: ret = (ret << 5) | decoded; /// @audit 5 277: uint bitlen = len * 5; /// @audit 8 278: if(len % 8 == 0) { /// @audit 5 280: ret = (ret << 5) | decoded; /// @audit 8 281: } else if(len % 8 == 2) { /// @audit 3 283: ret = (ret << 3) | (decoded >> 2); /// @audit 8 /// @audit 4 285: } else if(len % 8 == 4) { /// @audit 4 287: ret = (ret << 1) | (decoded >> 4); /// @audit 4 288: bitlen -= 4; /// @audit 8 /// @audit 5 289: } else if(len % 8 == 5) { /// @audit 4 291: ret = (ret << 4) | (decoded >> 1); /// @audit 8 /// @audit 7 293: } else if(len % 8 == 7) { /// @audit 3 295: ret = (ret << 2) | (decoded >> 3); /// @audit 3 296: bitlen -= 3; /// @audit 256 301: return bytes32(ret << (256 - bitlen));
File: contracts/dnssec-oracle/DNSSECImpl.sol /// @audit 3 241: if(dnskey.protocol != 3) {
File: contracts/dnssec-oracle/RRUtils.sol /// @audit 4 154: off += 4; /// @audit 8192 307: require(data.length <= 8192, "Long keys not permitted"); /// @audit 31 /// @audit 32 310: for(uint i = 0; i < data.length + 31; i += 32) { /// @audit 32 315: if(i + 32 > data.length) { /// @audit 256 /// @audit 8 316: uint unused = 256 - (data.length - i) * 8; /// @audit 0xFF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00 /// @audit 8 319: ac1 += (word & 0xFF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00) >> 8; /// @audit 0x00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF 320: ac2 += (word & 0x00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF); /// @audit 0x0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF 322: ac1 = (ac1 & 0x0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF) /// @audit 0xFFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000 /// @audit 16 323: + ((ac1 & 0xFFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000) >> 16); /// @audit 0x0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF 324: ac2 = (ac2 & 0x0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF) /// @audit 0xFFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000 /// @audit 16 325: + ((ac2 & 0xFFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000FFFF0000) >> 16); /// @audit 8 326: ac1 = (ac1 << 8) + ac2; /// @audit 0x00000000FFFFFFFF00000000FFFFFFFF00000000FFFFFFFF00000000FFFFFFFF 327: ac1 = (ac1 & 0x00000000FFFFFFFF00000000FFFFFFFF00000000FFFFFFFF00000000FFFFFFFF) /// @audit 0xFFFFFFFF00000000FFFFFFFF00000000FFFFFFFF00000000FFFFFFFF00000000 /// @audit 32 328: + ((ac1 & 0xFFFFFFFF00000000FFFFFFFF00000000FFFFFFFF00000000FFFFFFFF00000000) >> 32); /// @audit 0x0000000000000000FFFFFFFFFFFFFFFF0000000000000000FFFFFFFFFFFFFFFF 329: ac1 = (ac1 & 0x0000000000000000FFFFFFFFFFFFFFFF0000000000000000FFFFFFFFFFFFFFFF) /// @audit 0xFFFFFFFFFFFFFFFF0000000000000000FFFFFFFFFFFFFFFF0000000000000000 /// @audit 64 330: + ((ac1 & 0xFFFFFFFFFFFFFFFF0000000000000000FFFFFFFFFFFFFFFF0000000000000000) >> 64); /// @audit 0x00000000000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF 331: ac1 = (ac1 & 0x00000000000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF) /// @audit 128 332: + (ac1 >> 128); /// @audit 16 /// @audit 0xFFFF 333: ac1 += (ac1 >> 16) & 0xFFFF;
File: contracts/dnssec-oracle/SHA1.sol /// @audit 0x40 9: let scratch := mload(0x40) /// @audit 0xFFFFFFFFFFFFFFC0 16: let totallen := add(and(add(len, 1), 0xFFFFFFFFFFFFFFC0), 64) /// @audit 0x6745230100EFCDAB890098BADCFE001032547600C3D2E1F0 20: let h := 0x6745230100EFCDAB890098BADCFE001032547600C3D2E1F0 /// @audit 0x80 40: case 1 { mstore8(add(scratch, sub(len, i)), 0x80) } /// @audit 0xFFFFFFFEFFFFFFFEFFFFFFFEFFFFFFFEFFFFFFFEFFFFFFFEFFFFFFFEFFFFFFFE /// @audit 0x80000000 /// @audit 0x0000000100000001000000010000000100000001000000010000000100000001 49: temp := or(and(mul(temp, 2), 0xFFFFFFFEFFFFFFFEFFFFFFFEFFFFFFFEFFFFFFFEFFFFFFFEFFFFFFFEFFFFFFFE), and(div(temp, 0x80000000), 0x0000000100000001000000010000000100000001000000010000000100000001)) /// @audit 0xFFFFFFFCFFFFFFFCFFFFFFFCFFFFFFFCFFFFFFFCFFFFFFFCFFFFFFFCFFFFFFFC /// @audit 0x40000000 /// @audit 0x0000000300000003000000030000000300000003000000030000000300000003 54: temp := or(and(mul(temp, 4), 0xFFFFFFFCFFFFFFFCFFFFFFFCFFFFFFFCFFFFFFFCFFFFFFFCFFFFFFFCFFFFFFFC), and(div(temp, 0x40000000), 0x0000000300000003000000030000000300000003000000030000000300000003)) /// @audit 0x100000000000000000000 /// @audit 0x10000000000 65: f := xor(div(x, 0x100000000000000000000), div(x, 0x10000000000)) /// @audit 0x1000000000000000000000000000000 66: f := and(div(x, 0x1000000000000000000000000000000), f) /// @audit 0x10000000000 67: f := xor(div(x, 0x10000000000), f) /// @audit 0x5A827999 68: k := 0x5A827999 /// @audit 0x1000000000000000000000000000000 /// @audit 0x100000000000000000000 72: f := xor(div(x, 0x1000000000000000000000000000000), div(x, 0x100000000000000000000)) /// @audit 0x10000000000 73: f := xor(div(x, 0x10000000000), f) /// @audit 0x6ED9EBA1 74: k := 0x6ED9EBA1 /// @audit 0x1000000000000000000000000000000 /// @audit 0x100000000000000000000 78: f := or(div(x, 0x1000000000000000000000000000000), div(x, 0x100000000000000000000)) /// @audit 0x10000000000 79: f := and(div(x, 0x10000000000), f) /// @audit 0x1000000000000000000000000000000 /// @audit 0x100000000000000000000 80: f := or(and(div(x, 0x1000000000000000000000000000000), div(x, 0x100000000000000000000)), f) /// @audit 0x8F1BBCDC 81: k := 0x8F1BBCDC /// @audit 0x1000000000000000000000000000000 /// @audit 0x100000000000000000000 85: f := xor(div(x, 0x1000000000000000000000000000000), div(x, 0x100000000000000000000)) /// @audit 0x10000000000 86: f := xor(div(x, 0x10000000000), f) /// @audit 0xCA62C1D6 87: k := 0xCA62C1D6 /// @audit 0x80000000000000000000000000000000000000000000000 /// @audit 0x1F 90: let temp := and(div(x, 0x80000000000000000000000000000000000000000000000), 0x1F) /// @audit 0x800000000000000000000000000000000000000 /// @audit 0xFFFFFFE0 91: temp := or(and(div(x, 0x800000000000000000000000000000000000000), 0xFFFFFFE0), temp) /// @audit 0xFFFFFFFF 93: temp := add(and(x, 0xFFFFFFFF), temp) /// @audit 0x100000000000000000000000000000000000000000000000000000000 95: temp := add(div(mload(add(scratch, mul(j, 4))), 0x100000000000000000000000000000000000000000000000000000000), temp) /// @audit 0x10000000000 /// @audit 0x10000000000000000000000000000000000000000 96: x := or(div(x, 0x10000000000), mul(temp, 0x10000000000000000000000000000000000000000)) /// @audit 0xFFFFFFFF00FFFFFFFF000000000000FFFFFFFF00FFFFFFFF /// @audit 0x4000000000000 /// @audit 0xC0000000 /// @audit 0x400000000000000000000 /// @audit 0x3FFFFFFF /// @audit 0x100000000000000000000 97: x := or(and(x, 0xFFFFFFFF00FFFFFFFF000000000000FFFFFFFF00FFFFFFFF), mul(or(and(div(x, 0x4000000000000), 0xC0000000), and(div(x, 0x400000000000000000000), 0x3FFFFFFF)), 0x100000000000000000000)) /// @audit 0xFFFFFFFF00FFFFFFFF00FFFFFFFF00FFFFFFFF00FFFFFFFF 100: h := and(add(h, x), 0xFFFFFFFF00FFFFFFFF00FFFFFFFF00FFFFFFFF00FFFFFFFF) /// @audit 0x100000000 /// @audit 0xFFFFFFFF00000000000000000000000000000000 /// @audit 0x1000000 /// @audit 0xFFFFFFFF000000000000000000000000 /// @audit 0x10000 /// @audit 0xFFFFFFFF0000000000000000 /// @audit 0x100 /// @audit 0xFFFFFFFF00000000 /// @audit 0xFFFFFFFF /// @audit 0x1000000000000000000000000 102: ret := mul(or(or(or(or(and(div(h, 0x100000000), 0xFFFFFFFF00000000000000000000000000000000), and(div(h, 0x1000000), 0xFFFFFFFF000000000000000000000000)), and(div(h, 0x10000), 0xFFFFFFFF0000000000000000)), and(div(h, 0x100), 0xFFFFFFFF00000000)), and(h, 0xFFFFFFFF)), 0x1000000000000000000000000)
File: contracts/ethregistrar/ETHRegistrarController.sol /// @audit 3 78: return name.strlen() >= 3; /// @audit 4 /// @audit 36 258: bytes32 txNamehash = bytes32(data[i][4:36]);
File: contracts/ethregistrar/StringUtils.sol /// @audit 0x80 16: if(b < 0x80) { /// @audit 0xE0 18: } else if (b < 0xE0) { /// @audit 0xF0 20: } else if (b < 0xF0) { /// @audit 3 21: i += 3; /// @audit 0xF8 22: } else if (b < 0xF8) { /// @audit 4 23: i += 4; /// @audit 0xFC 24: } else if (b < 0xFC) { /// @audit 5 25: i += 5; /// @audit 6 27: i += 6;
File: contracts/registry/ReverseRegistrar.sol /// @audit 0x0 35: if (address(oldRegistrar) != address(0x0)) { /// @audit 0xf 170: mstore8(i, byte(and(addr, 0xf), lookup)) /// @audit 0x10 171: addr := div(addr, 0x10) /// @audit 0xf 173: mstore8(i, byte(and(addr, 0xf), lookup)) /// @audit 0x10 174: addr := div(addr, 0x10)
File: contracts/wrapper/ERC1155Fuse.sol /// @audit 192 143: expiry = uint64(t >> 192); /// @audit 160 147: fuses = uint32(t >> 160); /// @audit 160 162: (uint256(fuses) << 160) | /// @audit 192 163: (uint256(expiry) << 192); /// @audit 0x0 256: emit TransferSingle(msg.sender, address(0x0), newOwner, tokenId, 1); /// @audit 0x0 270: _setData(tokenId, address(0x0), 0, 0); /// @audit 0x0 271: emit TransferSingle(msg.sender, owner, address(0x0), tokenId, 1);
File: contracts/wrapper/NameWrapper.sol /// @audit 255 749: if (bytes(label).length > 255) { /// @audit 0x0 919: if (newOwner == address(0x0) || newOwner == address(this)) {
The type of the variable is the same as the type to which the variable is being cast
There is 1 instance of this issue:
File: contracts/registry/ReverseRegistrar.sol /// @audit address(resolver) 53: address(resolver) != address(0),
Events help non-contract tools to track changes, and events prevent users from being surprised by changes
There are 3 instances of this issue:
File: contracts/dnssec-oracle/Owned.sol 18 function setOwner(address newOwner) public owner_only { 19 owner = newOwner; 20: }
File: contracts/wrapper/NameWrapper.sol 105 function setMetadataService(IMetadataService _newMetadataService) 106 public 107 onlyOwner 108 { 109 metadataService = _newMetadataService; 110: } 128 function setUpgradeContract(INameWrapperUpgrade _upgradeAddress) 129 public 130 onlyOwner 131 { 132 if (address(upgradeContract) != address(0)) { 133 registrar.setApprovalForAll(address(upgradeContract), false); 134 ens.setApprovalForAll(address(upgradeContract), false); 135 } 136 137 upgradeContract = _upgradeAddress; 138 139 if (address(upgradeContract) != address(0)) { 140 registrar.setApprovalForAll(address(upgradeContract), true); 141 ens.setApprovalForAll(address(upgradeContract), true); 142 } 143: }
There is 1 instance of this issue:
File: contracts/ethregistrar/IBaseRegistrar.sol 0: import "../registry/ENS.sol";
Use a solidity version of at least 0.8.12 to get string.concat() to be used instead of abi.encodePacked(<str>,<str>)
There are 4 instances of this issue:
File: contracts/ethregistrar/ETHRegistrarController.sol 1: pragma solidity >=0.8.4;
File: contracts/registry/ReverseRegistrar.sol 1: pragma solidity >=0.8.4;
File: contracts/wrapper/BytesUtil.sol 2: pragma solidity >=0.8.4;
File: contracts/wrapper/NameWrapper.sol 2: pragma solidity ^0.8.4;
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
There are 3 instances of this issue:
File: contracts/dnssec-oracle/DNSSECImpl.sol 2: pragma solidity ^0.8.4;
File: contracts/dnssec-oracle/RRUtils.sol 1: pragma solidity ^0.8.4;
File: contracts/wrapper/ERC1155Fuse.sol 2: pragma solidity ^0.8.4;
pragma experimental ABIEncoderV2 is deprecatedUse pragma abicoder v2 instead
There are 2 instances of this issue:
File: contracts/dnssec-oracle/DNSSECImpl.sol 3: pragma experimental ABIEncoderV2;
File: contracts/dnssec-oracle/DNSSEC.sol 3: pragma experimental ABIEncoderV2;
Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync.
There is 1 instance of this issue:
File: contracts/wrapper/NameWrapper.sol /// @audit seen in contracts/registry/ReverseRegistrar.sol 35: ENS public immutable override ens;
Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file
There are 2 instances of this issue:
File: contracts/wrapper/NameWrapper.sol 45: //A contract address to a new upgraded contract if any 699: //check if it's the eth registrar ERC721
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length
There are 8 instances of this issue:
File: contracts/dnssec-oracle/BytesUtils.sol 252: bytes constant base32HexTable = hex'00010203040506070809FFFFFFFFFFFFFF0A0B0C0D0E0F101112131415161718191A1B1C1D1E1FFFFFFFFFFFFFFFFFFFFF0A0B0C0D0E0F101112131415161718191A1B1C1D1E1F';
File: contracts/dnssec-oracle/SHA1.sol 48: let temp := xor(xor(mload(add(scratch, sub(j, 12))), mload(add(scratch, sub(j, 32)))), xor(mload(add(scratch, sub(j, 56))), mload(add(scratch, sub(j, 64))))) 49: temp := or(and(mul(temp, 2), 0xFFFFFFFEFFFFFFFEFFFFFFFEFFFFFFFEFFFFFFFEFFFFFFFEFFFFFFFEFFFFFFFE), and(div(temp, 0x80000000), 0x0000000100000001000000010000000100000001000000010000000100000001)) 53: let temp := xor(xor(mload(add(scratch, sub(j, 24))), mload(add(scratch, sub(j, 64)))), xor(mload(add(scratch, sub(j, 112))), mload(add(scratch, sub(j, 128))))) 54: temp := or(and(mul(temp, 4), 0xFFFFFFFCFFFFFFFCFFFFFFFCFFFFFFFCFFFFFFFCFFFFFFFCFFFFFFFCFFFFFFFC), and(div(temp, 0x40000000), 0x0000000300000003000000030000000300000003000000030000000300000003)) 97: x := or(and(x, 0xFFFFFFFF00FFFFFFFF000000000000FFFFFFFF00FFFFFFFF), mul(or(and(div(x, 0x4000000000000), 0xC0000000), and(div(x, 0x400000000000000000000), 0x3FFFFFFF)), 0x100000000000000000000)) 102: ret := mul(or(or(or(or(and(div(h, 0x100000000), 0xFFFFFFFF00000000000000000000000000000000), and(div(h, 0x1000000), 0xFFFFFFFF000000000000000000000000)), and(div(h, 0x10000), 0xFFFFFFFF0000000000000000)), and(div(h, 0x100), 0xFFFFFFFF00000000)), and(h, 0xFFFFFFFF)), 0x1000000000000000000000000)
File: contracts/wrapper/ERC1155Fuse.sol 10: /* This contract is a variation on ERC1155 with the additions of _setData, getData and _canTransfer and ownerOf. _setData and getData allows the use of the other 96 bits next to the address of the owner for extra data. We use this to store 'fuses' that control permissions that can be burnt. 32 bits are used for the fuses themselves and 64 bits are used for the expiry of the name. When a name has expired, its fuses will be be set back to 0 */
Some files use >=, some use ^. The instances below are examples of the method that has the fewest instances for a specific version. Note that using >= without also specifying <= will lead to failures to compile, or external project incompatability, when the major version changes and there are breaking-changes, so ^ should be preferred regardless of the instance counts
There are 10 instances of this issue:
File: contracts/dnssec-oracle/SHA1.sol 1: pragma solidity >=0.8.4;
File: contracts/ethregistrar/ETHRegistrarController.sol 1: pragma solidity >=0.8.4;
File: contracts/ethregistrar/IETHRegistrarController.sol 1: pragma solidity >=0.8.4;
File: contracts/ethregistrar/StringUtils.sol 1: pragma solidity >=0.8.4;
File: contracts/registry/ENS.sol 1: pragma solidity >=0.8.4;
File: contracts/registry/IReverseRegistrar.sol 1: pragma solidity >=0.8.4;
File: contracts/registry/ReverseRegistrar.sol 1: pragma solidity >=0.8.4;
File: contracts/resolvers/Resolver.sol 2: pragma solidity >=0.8.4;
File: contracts/wrapper/BytesUtil.sol 2: pragma solidity >=0.8.4;
File: contracts/wrapper/IMetadataService.sol 1: pragma solidity >=0.8.4;
There are 4 instances of this issue:
File: contracts/dnssec-oracle/DNSSECImpl.sol 2: pragma solidity ^0.8.4;
File: contracts/dnssec-oracle/Owned.sol 1: pragma solidity ^0.8.4;
File: contracts/wrapper/Controllable.sol 2: pragma solidity ^0.8.4;
File: contracts/wrapper/NameWrapper.sol 2: pragma solidity ^0.8.4;
There are 5 instances of this issue:
File: contracts/wrapper/NameWrapper.sol /// @audit PARENT_CANOT_CONTROL 370: * @param fuses fuses to burn (cannot burn PARENT_CANOT_CONTROL) /// @audit regsitry 534: * @param ttl ttl in the regsitry /// @audit canCreateSubdomain /// @audit canReplaceSubdomain 649: * @dev Checks both canCreateSubdomain and canReplaceSubdomain and whether not they have been burnt /// @audit PARENT_CANNOT_REPLACE 893: // Set PARENT_CANNOT_REPLACE to reflect wrapper + registrar control over the 2LD
There are 14 instances of this issue:
File: contracts/dnssec-oracle/algorithms/Algorithm.sol 0: pragma solidity ^0.8.4;
File: contracts/dnssec-oracle/BytesUtils.sol 0: pragma solidity ^0.8.4;
File: contracts/dnssec-oracle/digests/Digest.sol 0: pragma solidity ^0.8.4;
File: contracts/dnssec-oracle/Owned.sol 0: pragma solidity ^0.8.4;
File: contracts/dnssec-oracle/RRUtils.sol 0: pragma solidity ^0.8.4;
File: contracts/dnssec-oracle/SHA1.sol 0: pragma solidity >=0.8.4;
File: contracts/ethregistrar/ETHRegistrarController.sol 0: pragma solidity >=0.8.4;
File: contracts/ethregistrar/IBaseRegistrar.sol 0: import "../registry/ENS.sol";
File: contracts/ethregistrar/IETHRegistrarController.sol 0: pragma solidity >=0.8.4;
File: contracts/ethregistrar/StringUtils.sol 0: pragma solidity >=0.8.4;
File: contracts/registry/ENS.sol 0: pragma solidity >=0.8.4;
File: contracts/registry/IReverseRegistrar.sol 0: pragma solidity >=0.8.4;
File: contracts/registry/ReverseRegistrar.sol 0: pragma solidity >=0.8.4;
File: contracts/wrapper/IMetadataService.sol 0: pragma solidity >=0.8.4;
There are 10 instances of this issue:
File: contracts/dnssec-oracle/DNSSEC.sol
File: contracts/dnssec-oracle/SHA1.sol
File: contracts/ethregistrar/IETHRegistrarController.sol
File: contracts/registry/ENS.sol
File: contracts/registry/IReverseRegistrar.sol
File: contracts/resolvers/Resolver.sol
File: contracts/wrapper/Controllable.sol
File: contracts/wrapper/IMetadataService.sol
File: contracts/wrapper/INameWrapper.sol
File: contracts/wrapper/INameWrapperUpgrade.sol
There are 13 instances of this issue:
File: contracts/dnssec-oracle/BytesUtils.sol /// @audit Missing: '@return' 232 * @param len The number of bytes to copy. 233 */ 234: function substring(bytes memory self, uint offset, uint len) internal pure returns(bytes memory) {
File: contracts/dnssec-oracle/DNSSECImpl.sol /// @audit Missing: '@return' 108 * @param now The current timestamp. 109 */ 110: function validateSignedSet(RRSetWithSignature memory input, bytes memory proof, uint256 now) internal view returns(RRUtils.SignedSet memory rrset) { /// @audit Missing: '@return' 145 * @param typecovered The type covered by the RRSIG record. 146 */ 147: function validateRRs(RRUtils.SignedSet memory rrset, uint16 typecovered) internal pure returns (bytes memory name) { /// @audit Missing: '@param rrset' 174 /** 175 * @dev Performs signature verification. 176 * 177 * Throws or reverts if unable to verify the record. 178 * 179 * @param name The name of the RRSIG record, in DNS label-sequence format. 180 * @param data The original data to verify. 181 * @param proof A DS or DNSKEY record that's already verified by the oracle. 182 */ 183: function verifySignature(bytes memory name, RRUtils.SignedSet memory rrset, RRSetWithSignature memory data, bytes memory proof) internal view { /// @audit Missing: '@param keyrdata' 226 /** 227 * @dev Attempts to verify some data using a provided key and a signature. 228 * @param dnskey The dns key record to verify the signature with. 229 * @param rrset The signed RRSET being verified. 230 * @param data The original data `rrset` was decoded from. 231 * @return True iff the key verifies the signature. 232 */ 233 function verifySignatureWithKey(RRUtils.DNSKEY memory dnskey, bytes memory keyrdata, RRUtils.SignedSet memory rrset, RRSetWithSignature memory data) 234 internal 235 view 236: returns (bool)
File: contracts/registry/ReverseRegistrar.sol /// @audit Missing: '@param resolver' 69 /** 70 * @dev Transfers ownership of the reverse ENS record associated with the 71 * calling account. 72 * @param addr The reverse record to set 73 * @param owner The address to set as the owner of the reverse record in ENS. 74 * @return The ENS node hash of the reverse record. 75 */ 76 function claimForAddr( 77 address addr, 78 address owner, 79 address resolver 80: ) public override authorised(addr) returns (bytes32) { /// @audit Missing: '@param resolver' 122 /** 123 * @dev Sets the `name()` record for the reverse ENS record associated with 124 * the account provided. First updates the resolver to the default reverse 125 * resolver if necessary. 126 * Only callable by controllers and authorised users 127 * @param addr The reverse record to set 128 * @param owner The owner of the reverse node 129 * @param name The name to set for this address. 130 * @return The ENS node hash of the reverse record. 131 */ 132 function setNameForAddr( 133 address addr, 134 address owner, 135 address resolver, 136 string memory name 137: ) public override returns (bytes32) {
File: contracts/wrapper/NameWrapper.sol /// @audit Missing: '@param tokenId' 112 /** 113 * @notice Get the metadata uri 114 * @return String uri of the metadata service 115 */ 116 117: function uri(uint256 tokenId) public view override returns (string memory) { /// @audit Missing: '@return' 207 * @param resolver resolver contract address 208 */ 209 210 function wrapETH2LD( 211 string calldata label, 212 address wrappedOwner, 213 uint32 fuses, 214 uint64 expiry, 215 address resolver 216: ) public override returns (uint64) { /// @audit Missing: '@param expiry' 264 /** 265 * @dev Renews a .eth second-level domain. 266 * Only callable by authorised controllers. 267 * @param tokenId The hash of the label to register (eg, `keccak256('foo')`, for 'foo.eth'). 268 * @param duration The number of seconds to renew the name for. 269 * @return expires The expiry date of the name on the .eth registrar, in seconds since the Unix epoch. 270 */ 271 function renew( 272 uint256 tokenId, 273 uint256 duration, 274 uint64 expiry 275: ) external override onlyController returns (uint256 expires) { /// @audit Missing: '@return' 370 * @param fuses fuses to burn (cannot burn PARENT_CANOT_CONTROL) 371 */ 372 373 function setFuses(bytes32 node, uint32 fuses) 374 public 375 onlyTokenOwner(node) 376 operationAllowed(node, CANNOT_BURN_FUSES) 377: returns (uint32) /// @audit Missing: '@param resolver' 393 /** 394 * @notice Upgrades a .eth wrapped domain by calling the wrapETH2LD function of the upgradeContract 395 * and burning the token of this contract 396 * @dev Can be called by the owner of the name in this contract 397 * @param label Label as a string of the .eth name to upgrade 398 * @param wrappedOwner The owner of the wrapped name 399 */ 400 401 function upgradeETH2LD( 402 string calldata label, 403 address wrappedOwner, 404: address resolver /// @audit Missing: '@return' 501 * @param expiry when the fuses will expire 502 */ 503 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)
indexed fieldsIndex event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (threefields). 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
There are 18 instances of this issue:
File: contracts/dnssec-oracle/DNSSEC.sol 14: event AlgorithmUpdated(uint8 id, address addr); 15: event DigestUpdated(uint8 id, address addr);
File: contracts/dnssec-oracle/SHA1.sol 4: event Debug(bytes32 x);
File: contracts/ethregistrar/ETHRegistrarController.sol 34 event NameRegistered( 35 string name, 36 bytes32 indexed label, 37 address indexed owner, 38 uint256 baseCost, 39 uint256 premium, 40 uint256 expires 41: ); 42 event NameRenewed( 43 string name, 44 bytes32 indexed label, 45 uint256 cost, 46 uint256 expires 47: );
File: contracts/ethregistrar/IBaseRegistrar.sol 9 event NameMigrated( 10 uint256 indexed id, 11 address indexed owner, 12 uint256 expires 13: ); 14 event NameRegistered( 15 uint256 indexed id, 16 address indexed owner, 17 uint256 expires 18: ); 19: event NameRenewed(uint256 indexed id, uint256 expires);
File: contracts/registry/ENS.sol 5: event NewOwner(bytes32 indexed node, bytes32 indexed label, address owner); 8: event Transfer(bytes32 indexed node, address owner); 11: event NewResolver(bytes32 indexed node, address resolver); 14: event NewTTL(bytes32 indexed node, uint64 ttl); 17 event ApprovalForAll( 18 address indexed owner, 19 address indexed operator, 20 bool approved 21: );
File: contracts/resolvers/Resolver.sol 35: event ContentChanged(bytes32 indexed node, bytes32 hash);
File: contracts/wrapper/Controllable.sol 9: event ControllerChanged(address indexed controller, bool active);
File: contracts/wrapper/INameWrapper.sol 19 event NameWrapped( 20 bytes32 indexed node, 21 bytes name, 22 address owner, 23 uint32 fuses, 24 uint64 expiry 25: ); 27: event NameUnwrapped(bytes32 indexed node, address owner); 29: event FusesSet(bytes32 indexed node, uint32 fuses, uint64 expiry);
Consider changing the variable to be an unnamed one
There are 4 instances of this issue:
File: contracts/dnssec-oracle/BytesUtils.sol /// @audit ret 135: function readUint8(bytes memory self, uint idx) internal pure returns (uint8 ret) {
File: contracts/dnssec-oracle/RRUtils.sol /// @audit ret 38: function readName(bytes memory self, uint offset) internal pure returns(bytes memory ret) {
File: contracts/wrapper/NameWrapper.sol /// @audit owner 90 function ownerOf(uint256 id) 91 public 92 view 93 override(ERC1155Fuse, INameWrapper) 94: returns (address owner) /// @audit ret 741 function _addLabel(string memory label, bytes memory name) 742 internal 743 pure 744: returns (bytes memory ret)
require()/revert() checks should be refactored to a modifier or functionThe compiler will inline the function, which will avoid JUMP instructions usually associated with functions
There are 6 instances of this issue:
File: contracts/dnssec-oracle/BytesUtils.sol 235: require(offset + len <= self.length);
File: contracts/wrapper/ERC1155Fuse.sol 199: require(to != address(0), "ERC1155: transfer to the zero address"); 290 require( 291 amount == 1 && oldOwner == from, 292 "ERC1155: insufficient balance for transfer" 293: ); 354: revert("ERC1155: ERC1155Receiver rejected tokens"); 357: revert(reason); 359: revert("ERC1155: transfer to non ERC1155Receiver implementer");
#0 - jefflau
2022-08-01T06:57:27Z
High quality submission documented well with links and code examples
#1 - dmvt
2022-10-14T09:13:45Z
[L-01] has been upgraded to medium and is a duplicate of #133
#2 - dmvt
2022-10-14T09:16:19Z
[L-03] Is actually invalid. The sponsor is not using the deprecated keyword now, but a variable which is passed into the functions in question.
#3 - dmvt
2022-10-14T09:20:34Z
[N-09] and [N-10] should be collapsed into one issue for brevity in the report.
#4 - dmvt
2022-10-14T09:23:07Z
[N-19] and [N-20] should both be considered Low Risk due to their being issues of incorrect or incomplete comments.
š Selected for report: 0xKitsune
Also found by: 0x040, 0x1f8b, 0x29A, 0xNazgul, 0xNineDec, 0xsam, 8olidity, Aussie_Battlers, Aymen0909, Bnke0x0, CRYP70, Ch_301, Chom, Deivitto, Dravee, ElKu, Fitraldys, Funen, GimelSec, IllIllI, JC, JohnSmith, Lambda, MiloTruck, Noah3o6, RedOneN, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, TomJ, Tomio, Waze, _Adam, __141345__, ajtra, ak1, arcoun, asutorufos, benbaessler, brgltd, bulej93, c3phas, cRat1st0s, cryptonue, delfin454000, durianSausage, fatherOfBlocks, gogo, hake, hyh, joestakey, karanctf, kyteg, lcfr_eth, lucacez, m_Rassska, rajatbeladiya, rbserver, robee, rokinot, sach1r0, sahar, samruna, sashik_eth, seyni, simon135, zuhaibmohd
159.1707 USDC - $159.17
| 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
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)
calldata instead of memory for read-only arguments in external functions saves gasWhen 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) {
File: contracts/dnssec-oracle/DNSSEC.sol /// @audit input 17: function verifyRRSet(RRSetWithSignature[] memory input) external virtual view returns(bytes memory);
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)))
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) {
File: contracts/wrapper/ERC1155Fuse.sol /// @audit onERC1155Received() 311: IERC1155Receiver(to).onERC1155Received( /// @audit onERC1155BatchReceived() 342: IERC1155Receiver(to).onERC1155BatchReceived(
internal functions only called once can be inlined to save gasNot 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) {
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
File: contracts/registry/ReverseRegistrar.sol 181: function ownsContract(address addr) internal view returns (bool) {
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
unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statementrequire(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);
<array>.length should not be looked up in every loop of a for-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD (3 gas)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++) {
File: contracts/ethregistrar/ETHRegistrarController.sol 256: for (uint256 i = 0; i < data.length; i++) {
File: contracts/wrapper/ERC1155Fuse.sol 92: for (uint256 i = 0; i < accounts.length; ++i) { 205: for (uint256 i = 0; i < ids.length; ++i) {
++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-loopsThe 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++) {
File: contracts/dnssec-oracle/DNSSECImpl.sol 93: for(uint i = 0; i < input.length; i++) {
File: contracts/ethregistrar/ETHRegistrarController.sol 256: for (uint256 i = 0; i < data.length; i++) {
File: contracts/ethregistrar/StringUtils.sol 14: for(len = 0; i < bytelength; len++) {
File: contracts/wrapper/ERC1155Fuse.sol 92: for (uint256 i = 0; i < accounts.length; ++i) { 205: for (uint256 i = 0; i < ids.length; ++i) {
require()/revert() strings longer than 32 bytes cost extra gasEach 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: );
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: );
File: contracts/wrapper/Controllable.sol 17: require(controllers[msg.sender], "Controllable: Caller is not a controller");
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");
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 {
File: contracts/dnssec-oracle/digests/Digest.sol /// @audit verify() 6: interface Digest {
File: contracts/dnssec-oracle/DNSSECImpl.sol /// @audit setAlgorithm(), setDigest() 16: contract DNSSECImpl is DNSSEC, Owned {
File: contracts/dnssec-oracle/DNSSEC.sol /// @audit verifyRRSet(), verifyRRSet() 5: abstract contract DNSSEC {
File: contracts/dnssec-oracle/Owned.sol /// @audit setOwner() 6: contract Owned {
File: contracts/ethregistrar/ETHRegistrarController.sol /// @audit valid(), withdraw() 17: contract ETHRegistrarController is Ownable, IETHRegistrarController {
File: contracts/ethregistrar/IBaseRegistrar.sol /// @audit addController(), removeController(), setResolver(), nameExpires(), available(), register(), renew(), reclaim() 5: interface IBaseRegistrar is IERC721 {
File: contracts/ethregistrar/IETHRegistrarController.sol /// @audit rentPrice(), available(), makeCommitment(), commit(), register(), renew() 5: interface IETHRegistrarController {
File: contracts/registry/ENS.sol /// @audit setRecord(), setSubnodeRecord(), setSubnodeOwner(), setResolver(), setOwner(), setTTL(), owner(), resolver(), ttl(), recordExists() 3: interface ENS {
File: contracts/registry/IReverseRegistrar.sol /// @audit setDefaultResolver(), claim(), claimForAddr(), claimWithResolver(), setName(), setNameForAddr(), node() 3: interface IReverseRegistrar {
File: contracts/registry/ReverseRegistrar.sol /// @audit setName() 8: abstract contract NameResolver {
File: contracts/resolvers/Resolver.sol /// @audit setABI(), setAddr(), setAddr(), setContenthash(), setDnsrr(), setName(), setPubkey(), setText(), setInterface(), multicall(), content(), multihash(), setContent(), setMultihash() 20: interface Resolver is
File: contracts/wrapper/Controllable.sol /// @audit setController() 6: contract Controllable is Ownable {
File: contracts/wrapper/ERC1155Fuse.sol /// @audit getData() 14: abstract contract ERC1155Fuse is ERC165, IERC1155, IERC1155MetadataURI {
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 {
File: contracts/wrapper/INameWrapperUpgrade.sol /// @audit setSubnodeRecord(), wrapETH2LD() 4: interface INameWrapperUpgrade {
File: contracts/wrapper/NameWrapper.sol /// @audit setMetadataService(), setUpgradeContract(), setFuses(), upgradeETH2LD(), upgrade(), setChildFuses(), setSubnodeOwner(), setSubnodeRecord() 27: contract NameWrapper is
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;
File: contracts/wrapper/ERC1155Fuse.sol 19: mapping(address => mapping(address => bool)) private _operatorApprovals;
++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++) {
File: contracts/dnssec-oracle/DNSSECImpl.sol 93: for(uint i = 0; i < input.length; i++) {
File: contracts/dnssec-oracle/RRUtils.sol 235: counts--; 241: othercounts--;
File: contracts/ethregistrar/ETHRegistrarController.sol 256: for (uint256 i = 0; i < data.length; i++) {
File: contracts/ethregistrar/StringUtils.sol 14: for(len = 0; i < bytelength; len++) {
require() statements that use && saves gasSee 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);
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: );
private rather than public for constants, saves gasIf 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;
require() or revert() statements that check input arguments should be at the top of the functionChecks 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);
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: );
revert()/require() strings to save gasCustom 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");
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: );
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: );
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");
File: contracts/wrapper/Controllable.sol 17: require(controllers[msg.sender], "Controllable: Caller is not a controller");
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: );
payableIf 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 {
File: contracts/wrapper/Controllable.sol 11: function setController(address controller, bool active) onlyOwner() public {
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)
#0 - jefflau
2022-08-01T07:00:10Z
[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.