Description

The Competition contract offers a way for the protocol to define performance metrics for the modelers based on their accuracy and results in previous contests. One of those performance metrics is reflected in the "top N Modelers" leaderboard, where the top performing modelers are displayed.

The replaceOptOutTopNModeler function allows the removal of a modeler from this leaderboard, under specific circumstances decided by the validators. The function is defined below:

function replaceOptOutTopNModeler(uint256 _oldModelerIndex, address _newModeler, uint256 _medianPerformanceResults) external onlyModelerContract {
    require(_oldModelerIndex < topNModelers.length, "ERR4");
    address _oldModeler = topNModelers[_oldModelerIndex];
    uint256 newIndex = findModelerUpperBound(_medianPerformanceResults);
    for (uint256 i = topNModelers.length; i > newIndex; i--) {
        topNModelers[i] = topNModelers[i - 1];
    }
    topNModelers[newIndex] = _newModeler;
    modelerToMedian[_newModeler] = _medianPerformanceResults;
    delete modelerToMedian[_oldModeler];
    emit TopNModelersUpdated(topNModelers);
}

As can be seen, the for loop tries accessing the topNModelers[topNModelers.length], which will revert, as Solidity is a zero-indexed programming language. On top of that, the modeler's index will be overwritten and not removed altogether.

Impact

Even though the code will compile, it will always revert because out-of-bounds accesses are handled by Solidity as exceptions.

Recommendations

We recommend assuring that the start of the for loop is at topNModelers.length - 1.

function replaceOptOutTopNModeler(uint256 _oldModelerIndex, address _newModeler, uint256 _medianPerformanceResults) external onlyModelerContract {
    require(_oldModelerIndex < topNModelers.length, "ERR4");
    address _oldModeler = topNModelers[_oldModelerIndex];
    uint256 newIndex = findModelerUpperBound(_medianPerformanceResults);
+   for (uint256 i = topNModelers.length - 1; i > newIndex; i--) {
        topNModelers[i] = topNModelers[i - 1];
    }
    topNModelers[newIndex] = _newModeler;
    modelerToMedian[_newModeler] = _medianPerformanceResults;
    delete modelerToMedian[_oldModeler];
    emit TopNModelersUpdated(topNModelers);
}

Moreover, we recommend covering for this particular function in the testing suite and assuring that no such similar issues are to occur in the future development of the protocol.

Remediation

This issue has been acknowledged by Spectral Finance, and a fix was implemented in commit 17d3d47a↗.