remove duplicated code and optimize (ExocoreNetwork#74)

* refactor(bootstrap): optimize checks

Instead of iterating through the validator set, use a mapping to
identify whether the consensus key and the name are unique.

Closes ExocoreNetwork#73 and ExocoreNetwork#18

* fix: replace ++i with i++

closes ExocoreNetwork#68

* refactor: DRY time check

Fixes ExocoreNetwork#21

* fix: validate cons key non-empty

* fix: use `block.timestamp >= lockTime`

The `isLocked` function returns true if `block.timestamp >= lockTime`,
which is exactly what the `_validateSpawnTimeAndOffsetDuration` function
should do.

The spawn time must not be in the past, or exactly at the current block.

The offset duration must be greater than equal to the spawn time,
however, if they are equal, the lock time ends up in the past, and it is
rejected.

* fix: init i = 0 in for loops

* fix: use correct var when emitting event

Use storage var instead of function param

* fix: don't touch test files in this PR

* test: add offset duration > spawn time test

Split from >= case since the errors are different

* refactor: remove duplicated code

script/generate.js

@@ -65,7 +65,7 @@ async function updateGenesisFile() {
     }
 
     // Set spawn time
-    const spawnTime = await myContract.methods.exocoreSpawnTime().call();
+    const spawnTime = await myContract.methods.spawnTime().call();
     const spawnTimeInSeconds = spawnTime.toString();
     const spawnDate = new Date(spawnTimeInSeconds * 1000).toISOString();
     genesisJSON.genesis_time = spawnDate;

src/core/Bootstrap.sol

@@ -68,26 +68,15 @@ contract Bootstrap is
         if (owner == address(0)) {
             revert Errors.ZeroAddress();
         }
-        if (spawnTime_ <= block.timestamp) {
-            revert Errors.BootstrapSpawnTimeAlreadyPast();
-        }
-        if (offsetDuration_ == 0) {
-            revert Errors.ZeroValue();
-        }
-        if (spawnTime_ <= offsetDuration_) {
-            revert Errors.BootstrapSpawnTimeLessThanDuration();
-        }
-        uint256 lockTime = spawnTime_ - offsetDuration_;
-        if (lockTime <= block.timestamp) {
-            revert Errors.BootstrapLockTimeAlreadyPast();
-        }
+
+        _validateSpawnTimeAndOffsetDuration(spawnTime_, offsetDuration_);
+        spawnTime = spawnTime_;
+        offsetDuration = offsetDuration_;
+
         if (customProxyAdmin_ == address(0)) {
             revert Errors.ZeroAddress();
         }
 
-        exocoreSpawnTime = spawnTime_;
-        offsetDuration = offsetDuration_;
-
         _addWhitelistTokens(whitelistTokens_);
 
         _whiteListFunctionSelectors[Action.REQUEST_MARK_BOOTSTRAP] = this.markBootstrapped.selector;
@@ -109,7 +98,7 @@ contract Bootstrap is
     /// @dev Returns true if the contract is locked, false otherwise.
     /// @return bool Returns `true` if the contract is locked, `false` otherwise.
     function isLocked() public view returns (bool) {
-        return block.timestamp >= exocoreSpawnTime - offsetDuration;
+        return block.timestamp >= spawnTime - offsetDuration;
     }
 
     /// @dev Modifier to restrict operations based on the contract's defined timeline, that is,
@@ -136,38 +125,49 @@ contract Bootstrap is
     /// @notice Allows the contract owner to modify the spawn time of the Exocore chain.
     /// @dev This function can only be called by the contract owner and must
     /// be called before the currently set lock time has started.
-    /// @param _spawnTime The new spawn time in seconds.
-    function setSpawnTime(uint256 _spawnTime) external onlyOwner beforeLocked {
-        if (_spawnTime <= block.timestamp) {
-            revert Errors.BootstrapSpawnTimeAlreadyPast();
-        }
-        if (_spawnTime <= offsetDuration) {
-            revert Errors.BootstrapSpawnTimeLessThanDuration();
-        }
-        uint256 lockTime = _spawnTime - offsetDuration;
-        if (lockTime <= block.timestamp) {
-            revert Errors.BootstrapLockTimeAlreadyPast();
-        }
+    /// @param spawnTime_ The new spawn time in seconds.
+    function setSpawnTime(uint256 spawnTime_) external onlyOwner beforeLocked {
+        _validateSpawnTimeAndOffsetDuration(spawnTime_, offsetDuration);
         // technically the spawn time can be moved backwards in time as well.
-        exocoreSpawnTime = _spawnTime;
-        emit SpawnTimeUpdated(_spawnTime);
+        spawnTime = spawnTime_;
+        emit SpawnTimeUpdated(spawnTime);
     }
 
     /// @notice Allows the contract owner to modify the offset duration that determines
     /// the lock period before the Exocore spawn time.
     /// @dev This function can only be called by the contract owner and must be called
     /// before the currently set lock time has started.
-    /// @param _offsetDuration The new offset duration in seconds.
-    function setOffsetDuration(uint256 _offsetDuration) external onlyOwner beforeLocked {
-        if (exocoreSpawnTime <= _offsetDuration) {
+    /// @param offsetDuration_ The new offset duration in seconds.
+    function setOffsetDuration(uint256 offsetDuration_) external onlyOwner beforeLocked {
+        _validateSpawnTimeAndOffsetDuration(spawnTime, offsetDuration_);
+        offsetDuration = offsetDuration_;
+        emit OffsetDurationUpdated(offsetDuration);
+    }
+
+    /// @dev Validates the spawn time and offset duration.
+    ///      The spawn time must be in the future and greater than the offset duration.
+    ///      The difference of the two must be greater than the current time.
+    /// @param spawnTime_ The spawn time of the Exocore chain to validate.
+    /// @param offsetDuration_ The offset duration before the spawn time to validate.
+    function _validateSpawnTimeAndOffsetDuration(uint256 spawnTime_, uint256 offsetDuration_) internal view {
+        if (offsetDuration_ == 0) {
+            revert Errors.ZeroValue();
+        }
+        // spawnTime_ == 0 is included in the below check, since the timestamp
+        // is always greater than 0. the spawn time must not be equal to the
+        // present time either, although, when marking as bootstrapped, we do
+        // allow that case intentionally.
+        if (block.timestamp > spawnTime_) {
+            revert Errors.BootstrapSpawnTimeAlreadyPast();
+        }
+        // guard against underflow of lockTime calculation
+        if (offsetDuration_ > spawnTime_) {
             revert Errors.BootstrapSpawnTimeLessThanDuration();
         }
-        uint256 lockTime = exocoreSpawnTime - _offsetDuration;
-        if (lockTime <= block.timestamp) {
+        uint256 lockTime = spawnTime_ - offsetDuration_;
+        if (block.timestamp >= lockTime) {
             revert Errors.BootstrapLockTimeAlreadyPast();
         }
-        offsetDuration = _offsetDuration;
-        emit OffsetDurationUpdated(_offsetDuration);
     }
 
     /// @inheritdoc ITokenWhitelister
@@ -182,7 +182,7 @@ contract Bootstrap is
     // like reentrancy.
     // slither-disable-next-line reentrancy-no-eth
     function _addWhitelistTokens(address[] calldata tokens) internal {
-        for (uint256 i; i < tokens.length; i++) {
+        for (uint256 i = 0; i < tokens.length; ++i) {
             address token = tokens[i];
             if (token == address(0)) {
                 revert Errors.ZeroAddress();
@@ -223,12 +223,12 @@ contract Bootstrap is
         if (bytes(validators[validatorAddress].name).length > 0) {
             revert Errors.BootstrapValidatorAlreadyRegistered();
         }
-        // check that the consensus key is unique.
-        if (consensusPublicKeyInUse(consensusPublicKey)) {
-            revert Errors.BootstrapConsensusPubkeyAlreadyUsed(consensusPublicKey);
+        _validateConsensusKey(consensusPublicKey);
+        // and that the name (meta info) is non-empty and unique.
+        if (bytes(name).length == 0) {
+            revert Errors.BootstrapValidatorNameLengthZero();
         }
-        // and that the name (meta info) is unique.
-        if (nameInUse(name)) {
+        if (validatorNameInUse[name]) {
             revert Errors.BootstrapValidatorNameAlreadyUsed();
         }
         // check that the commission is valid.
@@ -239,28 +239,11 @@ contract Bootstrap is
         validators[validatorAddress] =
             IValidatorRegistry.Validator({name: name, commission: commission, consensusPublicKey: consensusPublicKey});
         registeredValidators.push(msg.sender);
+        consensusPublicKeyInUse[consensusPublicKey] = true;
+        validatorNameInUse[name] = true;
         emit ValidatorRegistered(msg.sender, validatorAddress, name, commission, consensusPublicKey);
     }
 
-    /// @notice Checks if the given consensus public key is already in use by any registered validator.
-    /// @dev Iterates over all validators to determine if the key is in use.
-    /// @param newKey The input key to check.
-    /// @return bool Returns `true` if the key is already in use, `false` otherwise.
-    function consensusPublicKeyInUse(bytes32 newKey) public view returns (bool) {
-        if (newKey == bytes32(0)) {
-            revert Errors.ZeroValue();
-        }
-        uint256 arrayLength = registeredValidators.length;
-        for (uint256 i = 0; i < arrayLength; i++) {
-            address ethAddress = registeredValidators[i];
-            string memory exoAddress = ethToExocoreAddress[ethAddress];
-            if (validators[exoAddress].consensusPublicKey == newKey) {
-                return true;
-            }
-        }
-        return false;
-    }
-
     /// @notice Checks if the provided commission is valid.
     /// @dev The commission's rates must be <= 1e18 (100%) and the rate must be <= maxRate and maxChangeRate.
     /// @param commission The commission to check.
@@ -274,30 +257,15 @@ contract Bootstrap is
                commission.maxChangeRate <= commission.maxRate;
     }
 
-    /// @notice Checks if the given name is already in use by any registered validator.
-    /// @dev Iterates over all validators to determine if the name is in use.
-    /// @param newName The input name to check.
-    /// @return bool Returns `true` if the name is already in use, `false` otherwise.
-    function nameInUse(string memory newName) public view returns (bool) {
-        uint256 arrayLength = registeredValidators.length;
-        for (uint256 i = 0; i < arrayLength; i++) {
-            address ethAddress = registeredValidators[i];
-            string memory exoAddress = ethToExocoreAddress[ethAddress];
-            if (keccak256(abi.encodePacked(validators[exoAddress].name)) == keccak256(abi.encodePacked(newName))) {
-                return true;
-            }
-        }
-        return false;
-    }
-
     /// @inheritdoc IValidatorRegistry
     function replaceKey(bytes32 newKey) external beforeLocked whenNotPaused {
         if (bytes(ethToExocoreAddress[msg.sender]).length == 0) {
             revert Errors.BootstrapValidatorNotExist();
         }
-        if (consensusPublicKeyInUse(newKey)) {
-            revert Errors.BootstrapConsensusPubkeyAlreadyUsed(newKey);
-        }
+        _validateConsensusKey(newKey);
+        bytes32 oldKey = validators[ethToExocoreAddress[msg.sender]].consensusPublicKey;
+        consensusPublicKeyInUse[oldKey] = false;
+        consensusPublicKeyInUse[newKey] = true;
         validators[ethToExocoreAddress[msg.sender]].consensusPublicKey = newKey;
         emit ValidatorKeyReplaced(ethToExocoreAddress[msg.sender], newKey);
     }
@@ -332,6 +300,20 @@ contract Bootstrap is
         emit ValidatorCommissionUpdated(newRate);
     }
 
+    /// @notice Validates a consensus key.
+    /// @dev The validation checks include non-empty key and uniqueness.
+    /// @param key The consensus key to validate.
+    function _validateConsensusKey(bytes32 key) internal view {
+        // check that the consensus key is not empty.
+        if (key == bytes32(0)) {
+            revert Errors.ZeroValue();
+        }
+        // check that the consensus key is unique.
+        if (consensusPublicKeyInUse[key]) {
+            revert Errors.BootstrapConsensusPubkeyAlreadyUsed(key);
+        }
+    }
+
     /// @inheritdoc ILSTRestakingController
     function deposit(address token, uint256 amount)
         external
@@ -556,7 +538,7 @@ contract Bootstrap is
         // nonce match, which requires that inbound nonce is uint64(1).
         // TSS checks are not super clear since they can be set by anyone
         // but at this point that does not matter since it is not fully implemented anyway.
-        if (block.timestamp < exocoreSpawnTime) {
+        if (block.timestamp < spawnTime) {
             revert Errors.BootstrapNotSpawnTime();
         }
         if (bootstrapped) {

src/core/ClientChainGateway.sol

@@ -90,11 +90,12 @@ contract ClientChainGateway is
         delete clientChainGatewayLogic;
         delete clientChainInitializationData;
         // no risk keeping these but they are cheap to clear.
-        delete exocoreSpawnTime;
+        delete spawnTime;
         delete offsetDuration;
-        // previously, we tried clearing the loops but it is too expensive.
+        // previously, we tried clearing the contents of these in loops but it is too expensive.
         delete depositors;
         delete registeredValidators;
+        // mappings cannot be deleted
     }
 
     /// @notice Pauses the contract.

src/core/ClientGatewayLzReceiver.sol

@@ -307,7 +307,7 @@ abstract contract ClientGatewayLzReceiver is PausableUpgradeable, OAppReceiverUp
             revert InvalidAddWhitelistTokensRequest(expectedLength, requestPayload.length);
         }
 
-        for (uint256 i; i < count; i++) {
+        for (uint256 i = 0; i < count; ++i) {
             uint256 start = i * TOKEN_ADDRESS_BYTES_LENGTH + 1;
             uint256 end = start + TOKEN_ADDRESS_BYTES_LENGTH;
             address token = address(bytes20(requestPayload[start:end]));

src/core/ExocoreGateway.sol

@@ -112,7 +112,7 @@ contract ExocoreGateway is
         if (!success) {
             revert Errors.ExocoreGatewayFailedToGetClientChainIds();
         }
-        for (uint256 i = 0; i < chainIndices.length; i++) {
+        for (uint256 i = 0; i < chainIndices.length; ++i) {
             uint32 chainIndex = chainIndices[i];
             if (!chainToBootstrapped[chainIndex]) {
                 _sendInterchainMsg(chainIndex, Action.REQUEST_MARK_BOOTSTRAP, "", true);
@@ -190,7 +190,7 @@ contract ExocoreGateway is
 
         bool success;
         bool updated;
-        for (uint256 i; i < tokens.length; i++) {
+        for (uint256 i = 0; i < tokens.length; ++i) {
             require(tokens[i] != bytes32(0), "ExocoreGateway: token cannot be zero address");
             require(tvlLimits[i] > 0, "ExocoreGateway: tvl limit should not be zero");
             require(bytes(names[i]).length != 0, "ExocoreGateway: name cannot be empty");

src/libraries/Errors.sol

@@ -100,6 +100,9 @@ library Errors {
     /// @dev Bootstrap: client chain initialization data is malformed
     error BootstrapClientChainDataMalformed();
 
+    /// @dev Bootstrap: validator name length is zero
+    error BootstrapValidatorNameLengthZero();
+
     //////////////////////////////////
     //  BootstrapLzReceiver Errors  //
     //////////////////////////////////

src/libraries/Merkle.sol

@@ -53,7 +53,7 @@ library Merkle {
             proof.length != 0, "Merkle.processInclusionProofSha256: proof length should be a non-zero multiple of 32"
         );
         bytes32[1] memory computedHash = [leaf];
-        for (uint256 i = 0; i < proof.length; i++) {
+        for (uint256 i = 0; i < proof.length; ++i) {
             bytes32[1] memory node = [proof[i]];
             if (index % 2 == 0) {
                 // if ith bit of index is 0, then computedHash is a left sibling
@@ -90,15 +90,15 @@ library Merkle {
         //create a layer to store the internal nodes
         bytes32[] memory layer = new bytes32[](numNodesInLayer);
         //fill the layer with the pairwise hashes of the leaves
-        for (uint256 i = 0; i < numNodesInLayer; i++) {
+        for (uint256 i = 0; i < numNodesInLayer; ++i) {
             layer[i] = sha256(abi.encodePacked(leaves[2 * i], leaves[2 * i + 1]));
         }
         //the next layer above has half as many nodes
         numNodesInLayer /= 2;
         //while we haven't computed the root
         while (numNodesInLayer != 0) {
             //overwrite the first numNodesInLayer nodes in layer with the pairwise hashes of their children
-            for (uint256 i = 0; i < numNodesInLayer; i++) {
+            for (uint256 i = 0; i < numNodesInLayer; ++i) {
                 layer[i] = sha256(abi.encodePacked(layer[2 * i], layer[2 * i + 1]));
             }
             //the next layer above has half as many nodes

src/libraries/ValidatorContainer.sol

@@ -56,9 +56,9 @@ library ValidatorContainer {
 
     function merkleizeValidatorContainer(bytes32[] calldata validatorContainer) internal pure returns (bytes32) {
         bytes32[] memory leaves = validatorContainer;
-        for (uint256 i; i < MERKLE_TREE_HEIGHT; i++) {
+        for (uint256 i = 0; i < MERKLE_TREE_HEIGHT; ++i) {
             bytes32[] memory roots = new bytes32[](leaves.length / 2);
-            for (uint256 j; j < leaves.length / 2; j++) {
+            for (uint256 j = 0; j < leaves.length / 2; ++j) {
                 roots[j] = sha256(abi.encodePacked(leaves[2 * j], leaves[2 * j + 1]));
             }
             leaves = roots;

src/libraries/WithdrawalContainer.sol

@@ -42,9 +42,9 @@ library WithdrawalContainer {
 
     function merkleizeWithdrawalContainer(bytes32[] calldata withdrawalContainer) internal pure returns (bytes32) {
         bytes32[] memory leaves = withdrawalContainer;
-        for (uint256 i; i < MERKLE_TREE_HEIGHT; i++) {
+        for (uint256 i = 0; i < MERKLE_TREE_HEIGHT; ++i) {
             bytes32[] memory roots = new bytes32[](leaves.length / 2);
-            for (uint256 j; j < leaves.length / 2; j++) {
+            for (uint256 j = 0; j < leaves.length / 2; ++j) {
                 roots[j] = sha256(abi.encodePacked(leaves[2 * j], leaves[2 * j + 1]));
             }
             leaves = roots;

src/storage/BootstrapStorage.sol

@@ -22,8 +22,8 @@ contract BootstrapStorage is GatewayStorage {
     // time and duration
     /// @notice A timestamp representing the scheduled spawn time of the Exocore chain, which influences the contract's
     /// operational restrictions.
-    /// @dev `offsetDuration` before `exocoreSpawnTime`, the contract freezes and most actions are prohibited.
-    uint256 public exocoreSpawnTime;
+    /// @dev `offsetDuration` before `spawnTime`, the contract freezes and most actions are prohibited.
+    uint256 public spawnTime;
 
     /// @notice The amount of time before the Exocore spawn time during which operations are restricted.
     /// @dev The duration before the Exocore spawn time during which most contract operations are locked.
@@ -138,6 +138,14 @@ contract BootstrapStorage is GatewayStorage {
     /// this contract exceeding the limit and leading to creation failure.
     BeaconProxyBytecode public immutable BEACON_PROXY_BYTECODE;
 
+    /// @notice Mapping to keep track of the consensus keys that have been used.
+    /// @dev A mapping of consensus keys to a boolean indicating whether the key has been used.
+    mapping(bytes32 consensusKey => bool used) public consensusPublicKeyInUse;
+
+    /// @notice Mapping to keep track of the validator names that have been used.
+    /// @dev A mapping of validator names to a boolean indicating whether the name has been used.
+    mapping(string name => bool used) public validatorNameInUse;
+
     /* -------------------------------------------------------------------------- */
     /*                                   Events                                   */
     /* -------------------------------------------------------------------------- */

src/storage/GatewayStorage.sol

@@ -70,7 +70,7 @@ contract GatewayStorage {
         if (stringBytes.length != 42) {
             return false;
         }
-        for (uint256 i = 0; i < EXO_ADDRESS_PREFIX.length; i++) {
+        for (uint256 i = 0; i < EXO_ADDRESS_PREFIX.length; ++i) {
             if (stringBytes[i] != EXO_ADDRESS_PREFIX[i]) {
                 return false;
             }