BENQI Ignite

The Ignite contract has a function called _deleteRegistration() which removes an expired validator node registration or a non-tokenized node registration of a privileged account.

When a node is deleted, the last record is transferred to the index of the to-be-deleted node and the last node is popped from the array. However, the registrationIndicesByNodeIdHash mapping (which holds the registrations array index of each node's ID hash) for the transferred last record isn't updated.

Ignite::_deleteRegistration()

function _deleteRegistration(
    string memory nodeId
) internal {
    bytes32 nodeIdHash = keccak256(abi.encodePacked(nodeId));
    uint registrationIndex =
        registrationIndicesByNodeIdHash[nodeIdHash];
    ...
    uint totalRegistrations = registrations.length;
    // Dedaub: The registrationIndicesByNodeIdHash doesn't get updated
    //         when moving the records resulting in a conflict in
    //         the two arrays
    if (totalRegistrations != 1) {
        registrations[registrationIndex] =
            registrations[totalRegistrations - 1];
    }
    registrations.pop();
    delete registrationIndicesByNodeIdHash[nodeIdHash];

    emit RegistrationDeleted(nodeId);
}

As a result, all calls for the transferred record will fail, causing a loss of funds and generally breaking the entire functionality of the protocol.

The register() function of the Ignite contract fetches the AVAX and the QI price each time a new validator node registration is requested.

Ignite::register()

function register(
    string memory nodeId,
    uint validationDuration
) external payable {
    ...
    // Dedaub: Here the function fetches the current AVAX and QI prices
    //         from the Chainlink Oracles
    (, int256 avaxPrice, , ,) = avaxPriceFeed.latestRoundData();
    (, int256 qiPrice, , ,) = qiPriceFeed.latestRoundData();
    ...
}

However, the Chainlink Oracles may be down or unresponsive for a period of time, resulting in outdated prices being fetched.

The Oracles return the time they were last updated along with the asset's price. Adding a check for this value could therefore prevent outdated prices from being used.

The Ignite contract has a function called register() which allows a user to register a new validator node by locking up QI tokens and optionally AVAX.

To ensure that the sender is able to get AVAX back when the validation period expires, the function makes a test call to the sender.

Ignite::register()

function register(
    string memory nodeId,
    uint validationDuration
) external payable {

    require(msg.value == 0 || (msg.value >= minimumAvaxDeposit &&
                               msg.value <= maximumAvaxDeposit &&
                               msg.value % 1e9 == 0),
            "Invalid value");

    // Dedaub: Here a test call is made to ensure that the caller can
    //         receive AVAX back passing the control to the caller
    (bool success, ) = msg.sender.call("");
    require(success);

    ...
}

However, this transfers the control back to the user, who can re-enter the function, as there are no re-entrancy guards in place.

Although we do not believe that a re-entrancy can be concretely exploited in the current version of the contract, it is not unlikely that future modifications of the code might introduce such a vulnerability. As a consequence, we recommend adding a guard or using a checks-effects-interactions pattern (with proper documentation to ensure that the pattern is followed in the future).

The Ignite contract inherits from AccessControlUpgradeable, ReentrancyGuardUpgradeable and PausableUpgradeable. However, the initializer() of the contract doesn't call their initializers.

Ignite::initialize()

function initialize(
    address _qi, address _veQi,
    address _veQiDepositProxyImplementationRegister,
    address _slashedTokenRecipient,
    address _avaxPriceFeed, address _qiPriceFeed,
    uint _minimumAvaxDeposit, uint _maximumAvaxDeposit
) initializer public {

    require(_slashedTokenRecipient != address(0), "Zero address");

    _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);

    qi = IERC20Upgradeable(_qi);
    veQi = _veQi;
    veQiDepositProxyImplementationRegister =
        _veQiDepositProxyImplementationRegister;
    slashedTokenRecipient = _slashedTokenRecipient;
    avaxPriceFeed = IPriceFeed(_avaxPriceFeed);
    qiPriceFeed = IPriceFeed(_qiPriceFeed);
    minimumAvaxDeposit = _minimumAvaxDeposit;
    maximumAvaxDeposit = _maximumAvaxDeposit;

    qiSlashPercentage = 5_000;
    maximumSubsidisationAmount = 500_000e18;
}

Some of the initializers may currently have an empty implementation, but this could change in a future version of the OpenZeppelin code, which could be used by an upgraded version of the aforementioned contract. This omission of the call could lead to a serious bug if such a change went unnoticed or was not followed by the introduction of a call to the initializers.

Furthermore, the initializers of the ReentrancyGuardUpgradeable and PausableUpgradeable already have an implementation that is never executed. In particular, this may not cause any problems in the current state of the code, but it could lead to bugs, as mentioned above, in future versions if new functionality is added.

At several places in the code, a check is performed to ensure that the element at the selected index has the correct nodeId. For example:

Ignite::redeemAfterExpiry()

function redeemAfterExpiry(
    string memory nodeId
) external nonReentrant whenNotPaused {
    bytes32 nodeIdHash = keccak256(abi.encodePacked(nodeId));
    Registration storage registration =
        registrations[registrationIndicesByNodeIdHash[nodeIdHash]];

    require(
        keccak256(abi.encodePacked(registration.nodeId)) == nodeIdHash,
        "Registration not found");
    ...
}

Note that it is not sufficient to check that nodeIdHash exists in registrationIndicesByNodeIdHash, since we cannot distinguish a non-existent nodeId from one that happens to exist at index 0.

The code could be significantly simplified by simply adding a dummy unused element in the registrations array and starting the real elements from index 1. This way, a simple check registrationIndicesByNodeIdHash[nodeIdHash] != 0 is enough, the element at that index is guaranteed to have the correct nodeId.

In fact, there is no reason to hash the nodeIds at all. We could simply have a mapping:

mapping(string => uint) public registrationIndicesByNodeId;

that directly maps nodeIds to indices, saving the gas of hashing.

The Ignite contract has a function called releaseLockedTokens() which is called when the validation period has expired for releasing the deposited tokens for withdrawal.

However, when a user hasn’t deposited AVAX upon registering his validator node and he has been slashed in the meantime, then some redundant calls are made that could be avoided for saving some gas.

Ignite::releaseLockedTokens()

function releaseLockedTokens(
    string memory nodeId,
    uint rewards,
    bool slash
) external payable whenNotPaused {
    require(hasRole(ROLE_RELEASE_LOCKED_TOKENS, msg.sender),
        "ROLE_RELEASE_LOCKED_TOKENS");
    require(!slash || rewards == 0,
        "Cannot reward and slash the same node");
    ...
    require(registration.avaxDepositAmount == 0 || slash || rewards > 0,
        "Non-slashed node with deposited AVAX must be rewarded");
    ...
    if (rewards > 0) {
        ...
    } else if (slash) {
        require(msg.value == registration.avaxDepositAmount,
            "Invalid value");

        registration.slashed = true;

        uint avaxSlashAmount = registration.avaxDepositAmount *
            registration.avaxSlashPercentage / 10_000;

        uint qiSlashAmount = registration.qiDepositAmount *
            registration.qiSlashPercentage / 10_000;
        // Dedaub: Here avaxSlashAmount could be checked if it's 0 to
        //         avoid a redundant storage update and a zero-value
        //         transfer
        minimumContractBalance += msg.value - avaxSlashAmount;
        (bool success, ) = slashedTokenRecipient.call{
            value: avaxSlashAmount }("");
        require(success);
        ...
    } else {
        require(msg.value == 0, "Invalid value");
    }
}

The Ignite contract has a function called setAvaxDepositRange() which changes the range of the AVAX a user can send upon registering his validator node.

However, there are no checks to ensure that the new minimumAvaxDeposit is less than or equal to the maximumAvaxDeposit. Even though this can easily be resolved it would make sense to add such check for avoiding any unwanted interruptions from the proper execution of the protocol.

Ignite::setAvaxDepositRange()

function setAvaxDepositRange(
    uint newMinimumAvaxDeposit,
    uint newMaximumAvaxDeposit
) external {
    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender),
        "DEFAULT_ADMIN_ROLE");
    uint oldMinimumAvaxDeposit = minimumAvaxDeposit;
    uint oldMaximumAvaxDeposit = maximumAvaxDeposit;
    // Dedaub: No check whether the
    //         newMinimumAvaxDeposit <= newMaximumAvaxDeposit
    minimumAvaxDeposit = newMinimumAvaxDeposit;
    maximumAvaxDeposit = newMaximumAvaxDeposit;

    emit AvaxDepositRangeUpdated(
        oldMinimumAvaxDeposit,
        minimumAvaxDeposit,
        oldMaximumAvaxDeposit,
        maximumAvaxDeposit
    );
}

The code can be compiled with Solidity 0.8.0 or higher. For deployment, we recommend no floating pragmas, but a specific version, to be confident about the baseline guarantees offered by the compiler. Version 0.8.0, in particular, has some known bugs, which we do not believe affect the correctness of the contracts.