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: 60/100
Findings: 2
Award: $118.84
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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
78.881 USDC - $78.88
* data, followed by a series of canonicalised RR records that the signature
Change canonicalised to canonicalized
The same typo (Authorises/ Authorizes, Authorised/authorised, or Unauthorized/unauthorised) occurs in all 20 lines referenced below.
Example: ReverseRegistrar.sol: L40:
modifier authorised(address addr) {
Change authorised to authorized. Similarly for other forms of the word. This assumes that a decision has been made to use standard American English throughout ENS.
ETHRegistrarController.sol: L257
// check first few bytes are namehash
Change check first to check that first
* @dev An optimised function to compute the sha3 of the lower-case
Change optimised to optimized
/* 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 */
Remove duplicate word be
* @param ttl ttl in the regsitry
Change regsitry to registry
For the sake of readability, comments over 79 characters, such as the one below, should wrap using multi-line comment syntax.
/* 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 */
Comments such as the one below that contain TODOs or similar language should be addressed and modified or removed
// TODO: Check key isn't expired, unless updating key itself
return variable not used when a function returnsThe existence of a named return variable that is never used (here, newFuses) is potentially perplexing
function setFuses(bytes32 node, uint32 fuses) external returns (uint32 newFuses);
@param statements* @param name The name of the RRSIG record, in DNS label-sequence format. * @param data The original data to verify. * @param proof A DS or DNSKEY record that's already verified by the oracle. */ function verifySignature(bytes memory name, RRUtils.SignedSet memory rrset, RRSetWithSignature memory data, bytes memory proof) internal view {
@param statement is missing for rrset
* @param dnskey The dns key record to verify the signature with. * @param rrset The signed RRSET being verified. * @param data The original data `rrset` was decoded from. * @return True iff the key verifies the signature. */ function verifySignatureWithKey(RRUtils.DNSKEY memory dnskey, bytes memory keyrdata, RRUtils.SignedSet memory rrset, RRSetWithSignature memory data) internal view returns (bool) { // TODO: Check key isn't expired, unless updating key itself
@param statement is missing for keyrdata. I've included the "TODO" comment here (which I mentioned earlier) which refers to the key
* @param addr The reverse record to set * @param owner The address to set as the owner of the reverse record in ENS. * @return The ENS node hash of the reverse record. */ function claimForAddr( address addr, address owner, address resolver
@param missing for resolver. The resolver @param is also missing in the two examples below:
ReverseRegistrar.sol: L127-136
* @param addr The reverse record to set * @param owner The owner of the reverse node * @param name The name to set for this address. * @return The ENS node hash of the reverse record. */ function setNameForAddr( address addr, address owner, address resolver, string memory name
* @param label Label as a string of the .eth name to upgrade * @param wrappedOwner The owner of the wrapped name */ function upgradeETH2LD( string calldata label, address wrappedOwner, address resolver
* @param tokenId The hash of the label to register (eg, `keccak256('foo')`, for 'foo.eth'). * @param duration The number of seconds to renew the name for. * @return expires The expiry date of the name on the .eth registrar, in seconds since the Unix epoch. */ function renew( uint256 tokenId, uint256 duration, uint64 expiry
@param missing for expiry
require revert stringA require message should be included to enable users to understand reason for failure
Recommendation: Add a brief (<= 32 char) explanatory message to each of the lines below
require(offset + len <= self.length);
require(idx + 2 <= self.length);
require(idx + 4 <= self.length);
require(idx + 32 <= self.length);
require(idx + 20 <= self.length);
require(len <= 32);
require(idx + len <= self.length);
require(offset + len <= self.length);
require(len <= 52);
require(char >= 0x30 && char <= 0x7A);
require(decoded <= 0x20);
require(msg.sender == owner);
ETHRegistrarController.sol: L57
require(_maxCommitmentAge > _minCommitmentAge);
ETHRegistrarController.sol: L121
require(commitments[commitment] + maxCommitmentAge < block.timestamp);
ETHRegistrarController.sol: L246
require(duration >= MIN_REGISTRATION_DURATION);
require(offset + len <= self.length);
#0 - dmvt
2022-09-08T10:42:19Z
Marking British English as a typo knowingly is a massive waste of everyone's time.
🌟 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
39.9648 USDC - $39.96
Require revert string is too longThe 22 require revert strings referenced below should be shortened to 32 characters or fewer to save gas or else consider using custom error codes (which could save even more gas)
Example:
ETHRegistrarController.sol: L99-102
require( resolver != address(0), "ETHRegistrarController: resolver is required when data is supplied" );
Additional long require strings:
ETHRegistrarController.sol: L137-140
ETHRegistrarController.sol: L196-199
ETHRegistrarController.sol: L232-235
ETHRegistrarController.sol: L238-241
ETHRegistrarController.sol: L242
ETHRegistrarController.sol: L259-262
Revert error string is too longThe revert strings below should be shortened to 32 characters or fewer to save gas, or else consider using custom errors
revert("ERC1155: ERC1155Receiver rejected tokens"); } } catch Error(string memory reason) { revert(reason); } catch { revert("ERC1155: transfer to non ERC1155Receiver implementer");
revert("ERC1155: ERC1155Receiver rejected tokens"); } } catch Error(string memory reason) { revert(reason); } catch { revert("ERC1155: transfer to non ERC1155Receiver implementer");
require functionSplitting such functions into separate require() statements (instead of using &&) saves gas
require(char >= 0x30 && char <= 0x7A);
Recommendation:
require(char >= 0x30, "Error message"); require(char <= 0x7A, "Error message");
The same require function with embedded && occurs in both sets of lines referenced below:
require( amount == 1 && oldOwner == from, "ERC1155: insufficient balance for transfer" );
Recommendation:
require(amount == 1, "Error message"); require(oldOwner == from, "Error message");
for loopSince calculating the array length costs gas, it's best to read the length of the array from memory before executing the loop
for (uint256 i = 0; i < accounts.length; ++i) { batchBalances[i] = balanceOf(accounts[i], ids[i]); }
Suggestion:
uint256 totalAccountsLength = accounts.length; for (uint256 i = 0; i < totalAccountsLength; ++i) { batchBalances[i] = balanceOf(accounts[i], ids[i]); }
Similarly for the following for loops:
ETHRegistrarController.sol: L256-267
++i instead of i++ to increase count in a for loopSince use of i++ costs more gas, it is better to use ++i in the for loop below:
ETHRegistrarController.sol: L256-267
for loopUnderflow/overflow checks are made every time ++i (or i++) is called. Such checks are unnecessary since i is already limited. Therefore, use unchecked{++i}/unchecked{i++} instead
for (uint256 i = 0; i < accounts.length; ++i) { batchBalances[i] = balanceOf(accounts[i], ids[i]); }
Suggestion:
uint256 totalAccountsLength = accounts.length; for (uint256 i = 0; i < totalAccountsLength;) { batchBalances[i] = balanceOf(accounts[i], ids[i]); unchecked{ ++i; } }
Similarly for the following for loops:
ETHRegistrarController.sol: L256-267