Merge pull request #180 from Consensys/fix/audit-4.5-overflow-nameWra…

…pper

audit 4.5 Overflow in the NameWrapper Contract

packages/l2-contracts/contracts/ethregistrar/ETHRegistrarController.sol

@@ -109,6 +109,13 @@ contract ETHRegistrarController is
 
     event NameRenewedPoh(string name, bytes32 indexed label, uint256 expires);
 
+    modifier minRegistrationDuration(uint256 duration) {
+        if (duration < MIN_REGISTRATION_DURATION) {
+            revert DurationTooShort(duration);
+        }
+        _;
+    }
+
     /**
      * @dev Ensures the address is not address(0).
      * @param _addr Address to check.
@@ -219,7 +226,7 @@ contract ETHRegistrarController is
         bytes[] calldata data,
         bool reverseRecord,
         uint16 ownerControlledFuses
-    ) public pure override returns (bytes32) {
+    ) public pure override minRegistrationDuration(duration) returns (bytes32) {
         bytes32 label = keccak256(bytes(name));
         if (data.length > 0 && resolver == address(0)) {
             revert ResolverRequiredWhenDataSupplied();
@@ -510,7 +517,7 @@ contract ETHRegistrarController is
         string memory name,
         uint256 duration,
         bytes32 commitment
-    ) internal {
+    ) internal minRegistrationDuration(duration) {
         // Require an old enough commitment.
         if (commitments[commitment] + minCommitmentAge > block.timestamp) {
             revert CommitmentTooNew(commitment);
@@ -525,10 +532,6 @@ contract ETHRegistrarController is
         }
 
         delete (commitments[commitment]);
-
-        if (duration < MIN_REGISTRATION_DURATION) {
-            revert DurationTooShort(duration);
-        }
     }
 
     /**

packages/l2-contracts/contracts/wrapper/NameWrapper.sol

@@ -13,6 +13,7 @@ import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Recei
 import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
 import {BytesUtils} from "./BytesUtils.sol";
 import {ERC20Recoverable} from "../utils/ERC20Recoverable.sol";
+import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";
 
 import "hardhat/console.sol";
 
@@ -341,7 +342,9 @@ contract NameWrapper is
         // transfer the ens record back to the new owner (this contract)
         registrar.reclaim(tokenId, address(this));
 
-        expiry = uint64(registrar.nameExpires(tokenId)) + GRACE_PERIOD;
+        expiry =
+            SafeCast.toUint64(registrar.nameExpires(tokenId)) +
+            GRACE_PERIOD;
 
         _wrap(label, wrappedOwner, ownerControlledFuses, expiry, resolver);
     }
@@ -366,11 +369,12 @@ contract NameWrapper is
     ) external onlyController returns (uint256 registrarExpiry) {
         uint256 tokenId = uint256(keccak256(bytes(label)));
         registrarExpiry = registrar.register(tokenId, address(this), duration);
+
         _wrap(
             label,
             wrappedOwner,
             ownerControlledFuses,
-            uint64(registrarExpiry) + GRACE_PERIOD,
+            SafeCast.toUint64(registrarExpiry) + GRACE_PERIOD,
             resolver
         );
     }
@@ -404,7 +408,7 @@ contract NameWrapper is
         }
 
         // Set expiry in Wrapper
-        uint64 expiry = uint64(registrarExpiry) + GRACE_PERIOD;
+        uint64 expiry = SafeCast.toUint64(registrarExpiry) + GRACE_PERIOD;
 
         // Use super to allow names expired on the wrapper, but not expired on the registrar to renew()
         (address owner, uint32 fuses, ) = super.getData(uint256(node));
@@ -928,7 +932,8 @@ contract NameWrapper is
         // transfer the ens record back to the new owner (this contract)
         registrar.reclaim(uint256(labelhash), address(this));
 
-        uint64 expiry = uint64(registrar.nameExpires(tokenId)) + GRACE_PERIOD;
+        uint64 expiry = SafeCast.toUint64(registrar.nameExpires(tokenId)) +
+            GRACE_PERIOD;
 
         _wrap(label, owner, ownerControlledFuses, expiry, resolver);
 

packages/l2-contracts/test/ethregistrar/TestEthRegistrarController.js

@@ -20,6 +20,7 @@ const {
   BASE_DOMAIN_STR,
   BASE_NODE_DNS_ENCODED,
   BASE_DOMAIN_LABEL,
+  OVER_MAX_REGISTRATION_DURATION,
 } = require('../test-utils/constants')
 
 const DAY = 24 * 60 * 60
@@ -934,6 +935,20 @@ contract('ETHRegistrarController', function () {
     ).to.equal(86400)
   })
 
+  it('should revert when renewing with a duration too long', async () => {
+    await registerName('newname')
+    // Max uint64 to go over the max limit duration
+    const [price] = await controller.rentPrice(
+      sha3('newname'),
+      OVER_MAX_REGISTRATION_DURATION,
+    )
+    await expect(
+      controller2.renew('newname', OVER_MAX_REGISTRATION_DURATION, {
+        value: price,
+      }),
+    ).to.be.revertedWith(`SafeCast: value doesn't fit in 64 bits`)
+  })
+
   it('should allow token owners to renew a name for free using POH', async () => {
     const name = 'newname'
     const duration = 3 * 365 * 24 * 60 * 60 // 3 years
@@ -1415,6 +1430,26 @@ contract('ETHRegistrarController', function () {
     await expect(tx).to.emit(controllerPoh, 'OwnerNameRegistered')
   })
 
+  it('should revert ownerRegister if duration too long', async function () {
+    const name = 'ownername'
+    const resolverAddress = resolver.address
+    const ownerControlledFuses = 0
+    const reverseRecord = false
+
+    await expect(
+      controllerPoh.ownerRegister(
+        name,
+        ownerAccount,
+        OVER_MAX_REGISTRATION_DURATION,
+        resolverAddress,
+        [], // data
+        ownerControlledFuses,
+        reverseRecord,
+        { from: ownerAccount },
+      ),
+    ).to.be.revertedWith(`SafeCast: value doesn't fit in 64 bits`)
+  })
+
   it('should revert if called by a non-owner', async function () {
     const name = 'nonownername'
     const duration = 28 * 24 * 60 * 60

packages/l2-contracts/test/test-utils/constants.js

@@ -16,6 +16,9 @@ const BASE_NODE_DNS_ENCODED = new TextEncoder().encode('\x05linea\x03eth\x00')
 
 const BASE_DOMAIN_LABEL = sha3('linea')
 
+// Max uint64 + 1
+const OVER_MAX_REGISTRATION_DURATION = '18446744073709551616'
+
 module.exports = {
   EMPTY_ADDRESS,
   EMPTY_BYTES32,
@@ -24,4 +27,5 @@ module.exports = {
   BASE_NODE_2_BYTES32,
   BASE_NODE_DNS_ENCODED,
   BASE_DOMAIN_LABEL,
+  OVER_MAX_REGISTRATION_DURATION,
 }

packages/l2-contracts/test/wrapper/NameWrapper.js

@@ -16,6 +16,7 @@ const {
   BASE_NODE_DNS_ENCODED,
   BASE_DOMAIN_STR,
   BASE_DOMAIN_LABEL,
+  OVER_MAX_REGISTRATION_DURATION,
 } = require('../test-utils/constants')
 
 const abiCoder = new ethers.utils.AbiCoder()
@@ -6625,6 +6626,31 @@ describe('Name Wrapper', () => {
       // still expired
       expect(expiryAfter).to.be.at.most(block1.timestamp + GRACE_PERIOD)
     })
+
+    it('should revert when registering with a duration too long', async () => {
+      await expect(
+        NameWrapper.registerAndWrap(
+          label,
+          account,
+          OVER_MAX_REGISTRATION_DURATION,
+          EMPTY_ADDRESS,
+          CAN_DO_EVERYTHING,
+        ),
+      ).to.be.revertedWith(`SafeCast: value doesn't fit in 64 bits`)
+    })
+
+    it('should revert when renewing with a duration too long', async () => {
+      await NameWrapper.registerAndWrap(
+        label,
+        account,
+        86400,
+        EMPTY_ADDRESS,
+        CAN_DO_EVERYTHING,
+      )
+      await expect(
+        NameWrapper.renew(labelHash, OVER_MAX_REGISTRATION_DURATION),
+      ).to.be.revertedWith(`SafeCast: value doesn't fit in 64 bits`)
+    })
   })
 
   describe('Controllable', () => {