Description

The registerValidator function facilitates the registration of validators, a privileged role within the Modeler contract.

function registerValidator() public whenNotPaused {
    // commented the library call for ease of reading
    //ModelerLibrary.registerValidator(
    //   validatorAddresses, 
    //   maxValidators, 
    //   validatorToken, 
    //   validators[msg.sender], 
    //   validatorStakeAmount, 
    //   isValidatorRegistered
    //);

    require(validatorAddresses.length < MAX_VALIDATORS, "Max validators reached");
    require(isValRegistered[msg.sender] == false || (validator.optOutTime > 0 && validator.optOutTime < block.timestamp - ONE_DAY) , "Validator already registered"); 
    validatorToken.safeTransferFrom(msg.sender, address(this), validatorStakeAmount);
    isValRegistered[msg.sender] = true;
    validator.currentStake = validatorStakeAmount;
    validator.optOutTime = ~uint256(0);
    addValidatorRewardList(msg.sender, validatorAddresses, isValRegistered);
}

The function checks whether the validatorAddresses.length < MAX_VALIDATORS and assures that the required amount of validator tokens is paid for by the to-be validator (i.e., msg.sender). It then sets the value of isValRegistered (a storage reference of isValidatorRegistered) to true, effectively giving the validator role to the msg.sender.

Technically all the checks so far are performing their intended role. The MAX_VALIDATORS would be set to 1 in the current version of the codebase, as stated by the Spectral team, which means that only the first to ever call the registerValidator will become validator.

Upon closer examination of the addValidatorRewardList function (last line in the registerValidator definition), we observe the following issue:

function addValidatorRewardList(address _validator, address[] storage validatorAddresses, mapping(address => bool) storage isValRegistered) internal {
    if (isValRegistered[_validator]) {
        return;
    }
    validatorAddresses.push(_validator);
}

Note that before calling addValidatorRewardList, the storage was previously updated isValRegistered[msg.sender] = true;. Thus, the if (isValRegistered[_validator]) will always hold true, and therefore the validatorAddresses.push(_validator); will never be called. This, in turn, means that the previous require(validatorAddresses.length < MAX_VALIDATORS, "Max validators reached"); check will always be redundant, allowing for an unlimited amount of users to call registerValidator with the only virtual constraint of affording the staking required to do so.

Impact

Anyone can call registerValidator and become a validator, the highest ranking role in the Modeler contract. The capabilities of a validator include the removal of other validators or modelers at will, changing the performance rating of modelers, overwriting existing challenges, and so forth.

Recommendations

We recommend moving the addValidatorRewardList before the isValRegistered storage change. For additional assurance, we highly recommend drastically limiting the access to this function, by means of setting an admin that would oversee the registration of validators.

We consider that this particular issue could have been readily identified with a basic testing suite and therefore would encourage the team to set up a comprehensive testing suite to help mitigate against similar issues in the future.

Remediation

The Spectral team has mitigated this issue in commit 2b6f0b9e↗ by performing a push operation into the validatorAddresses straight away, rather than checking whether the value is already registered as a validator or not. This solution does address the issue, on the assumption that the require(validatorAddresses.length < MAX_VALIDATORS, "Max validators reached"); holds. No additional access control modifiers have been added to this function.