ENS contest - brgltd's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L819-L821

Vulnerability details

Impact

An external call "_transfer" is made before updating state data through "_setFuses" and "_setFuses" does not depend on any data from "_transfer".

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L819-L821.

Proof of Concept

Reentrancy is not only an effect of Ether transfer but of any function call on another contract.

The contract should make use of the Checks-Effects-Interactions Pattern.

Recommended Mitigation Steps

Update the state before making the external call.

$ git diff --no-index NameWrapper.sol.orig NameWrapper.sol
diff --git a/NameWrapper.sol.orig b/NameWrapper.sol
index b652447..d0dfa6f 100644
--- a/NameWrapper.sol.orig
+++ b/NameWrapper.sol
@@ -817,8 +817,8 @@ contract NameWrapper is
         uint64 expiry
     ) internal {
         (address owner, , ) = getData(uint256(node));
-        _transfer(owner, newOwner, uint256(node), 1, "");
         _setFuses(node, newOwner, fuses, expiry);
+        _transfer(owner, newOwner, uint256(node), 1, "");
     }

[L-01] BYTESUTILS.SOL IS THE SAME NAME FOR TWO CONTRACTS

The following files have the same contract name

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

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

The compilation artifacts will not contain one of the contracts with the duplicate name. Therefore, only one of the two contracts will generate artifacts.

[NC-01] VARIABLE SHADOWS BUILT-IN SYMBOL.

Variables should not shadow with the built-in symbols, and should be refactored to avoid conflicts with the symbols.

There are 3 instances of this issue with the parameter "now".

File: contracts/dnssec-oracle/DNSSEC.sol
18: function verifyRRSet(RRSetWithSignature[] memory input, uint256 now) public view virtual returns(bytes memory);

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

File: contracts/dnssec-oracle/DNSSECImpl.sol
91: function verifyRRSet(RRSetWithSignature[] memory input, uint256 now) public virtual view override returns(bytes memory) {

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

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

[NC-02] EMIT AN EVENT FOR CRITICAL PARAMETERS CHANGE

The lack of an event will make it difficult to track off-chain changes for the "setOwner" function.

File: contracts/dnssec-oracle/Owned.sol
19: owner = newOwner;

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

[NC-03] REMOVE UNUSED PRIVATE/INTERNAL FUNCTIONS

There are 2 instances of this issue.

File: contracts/dnssec-oracle/BytesUtils.sol
198: function readBytesN(bytes memory self, uint idx, uint len) internal pure returns (bytes32 ret) {

207: function memcpy(uint dest, uint src, uint len) private pure {

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

[NC-04] MISSING INHERITANCE

ETHRegistrarController should inherit from IERC165.

File: contracts/ethregistrar/ETHRegistrarController.sol
17: contract ETHRegistrarController is Ownable, IETHRegistrarController {

220: interfaceID == type(IERC165).interfaceId ||

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

[NC-05] FUNCTIONS NOT CALLED BY THE CONTRACT SHOULD BE DECLARED EXTERNAL INSTEAD OF PUBLIC

There are 22 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 {

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

File: contracts/dnssec-oracle/Owned.sol
18: fUnction setOwner(address newOwner) public owner_only {

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

File: contracts/ethregistrar/ETHRegistrarController.sol
120: function commit(bytes32 commitment) public override {

210: function withdraw() public {

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

File: contracts/wrapper/ERC1155Fuse.sol
78: function balanceOfBatch(address[] memory accounts, uint256[] memory ids)

102: function setApprovalForAll(address operator, bool approved)

169: function safeTransferFrom(

188: function safeBatchTransferFrom(

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

File: contracts/wrapper/NameWrapper.sol
105: function setMetadataService(IMetadataService _newMetadataService)

117: function uri(uint256 tokenId) public view override returns (string memory) {

128: function setUpgradeContract(INameWrapperUpgrade _upgradeAddress)

210: function wrapETH2LD(

295: function wrap(

335: function unwrapETH2LD(

356: function unwrap(

401: function upgradeETH2LD(

429: function upgrade(

584: function setRecord(

609: function setResolver(bytes32 node, address resolver)

624: function setTTL(bytes32 node, uint64 ttl)

693: function onERC721Received(

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

[G-01] ++I/--I COSTS LESS GAS THAN I++/I--, ESPECIALLY FOR LOOPS

Saves about 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/main/contracts/dnssec-oracle/BytesUtils.sol

File: contracts/dnssec-oracle/DNSSECImpl.sol
93: for(uint i = 0; i < input.length; i++) {

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

File: contracts/ethregistrar/ETHRegistrarController.sol
256: for (uint256 i = 0; i < data.length; i++) {

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

File: contracts/ethregistrar/StringUtils.sol
14: for(len = 0; i < bytelength; len++) {

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

File: contracts/dnssec-oracle/RRUtils.sol
235: counts--;

241: othercounts--;

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

[G-02] THE INCREMENT IN FOR LOOP POST CONDITION CAN BE MADE UNCHECKED

The solidity compiler will apply arithmetic checks for the increment step during for loops. This can be disabled since the value being incremented won't surpass the upper bound that's checked on the break condition.

Adding uncheck can save 30-40 gas per loop.

There are 10 instances of this issue.

File: contracts/dnssec-oracle/BytesUtils.sol
56: for (uint idx = 0; idx < shortest; idx += 32) {

209: for (; len >= 32; len -= 32) {

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/main/contracts/dnssec-oracle/BytesUtils.sol

File: contracts/dnssec-oracle/DNSSECImpl.sol
93: for(uint i = 0; i < input.length; i++) {

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

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

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

File: contracts/ethregistrar/StringUtils.sol
14: for(len = 0; i < bytelength; len++) {

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

File: contracts/dnssec-oracle/RRUtils.sol
310: for(uint i = 0; i < data.length + 31; i += 32) {

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

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/main/contracts/wrapper/ERC1155Fuse.sol

[G-03] ARRAY.LENGTH SHOULD NOT BE COMPUTED ON EVERY INTERATION DURING A LOOP

Instead of computing array.length for every iteration, the value for array.length should be cached before the loop to save gas.

There are 5 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/main/contracts/dnssec-oracle/DNSSECImpl.sol

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

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

File: contracts/dnssec-oracle/RRUtils.sol
310: for(uint i = 0; i < data.length + 31; i += 32) {

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

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/main/contracts/wrapper/ERC1155Fuse.sol

[G-04] USE CUSTOM ERRORS RATHER THAN REVERT()/REQUIRE() TO SAVE GAS

Custom errors reduce the cost to deploy and call a function.

Requires are expensive, specially with strings.

Using custom error would save gas and ensure compatibility with the latest best practices in solidity.

There are 45 instances of this issue.

File: contracts/dnssec-oracle/BytesUtils.sol

12: require(offset + len <= self.length);

146: require(offset + len <= 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();

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

File: contracts/dnssec-oracle/Owned.sol
10: require(msg.sender == owner);

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

File: contracts/dnssec-oracle/RRUtils.sol
307: require(data.length <= 8192, "Long keys not permitted");

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

File: contracts/ethregistrar/ETHRegistrarController.sol
57: require(_maxCommitmentAge > _minCommitmentAge);

99: require(

121: require(commitments[commitment] + maxCommitmentAge < block.timestamp);

137: require(

196: require(

232: require(

238: require(

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

259: require(

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

File: contracts/registry/ReverseRegistrar.sol
41: require(

52: require(

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

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/main/contracts/wrapper/Controllable.sol

File: contracts/wrapper/ERC1155Fuse.sol
60: require(

85: require(

107: require(

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

177: require(

195: require(

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

200: require(

215: require(

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

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

250: require(

290: require(

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

325: revert(reason);

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

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

357: revert(reason);

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

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

[G-05] IT COSTS MORE GAS TO INITIALIZE VARIABLES TO ZERO THAN TO LET THE DEFAULT OF ZERO BE APPLIED

File: contracts/dnssec-oracle/BytesUtils.sol
56: for (uint idx = 0; idx < shortest; idx += 32) {

264: uint ret = 0;

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

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

File: contracts/dnssec-oracle/DNSSECImpl.sol
93: for(uint i = 0; i < input.length; i++) {

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

File: contracts/dnssec-oracle/RRUtils.sol
50: uint count = 0;

310: for(uint i = 0; i < data.length + 31; i += 32) {

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

File: contracts/ethregistrar/ETHRegistrarController.sol
256: for (uint256 i = 0; i < data.length; i++) {

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

File: contracts/ethregistrar/StringUtils.sol
12: uint i = 0;

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

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

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/main/contracts/wrapper/ERC1155Fuse.sol

[G-06] NOT USING THE NAMED RETURN VARIABLES WHEN A FUNCTION RETURNS, WASTES DEPLOYMENT GAS

File: contracts/dnssec-oracle/BytesUtils.sol
136: return uint8(self[idx]);

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