Description

The removeSplitter function removes a splitter. Since the splitters are recorded in an array, it loops through the array in order to locate the splitter and then replaces it with the last splitter:

function removeSplitter(address _account) external onlyOwner {
    // [...]
    uint256 numAccounts = accounts.length;
    for (uint256 i = 0; i < numAccounts; ++i) {
        if (accounts[i] == _account) {
            accounts[i] = accounts[numAccounts - 1];
            accounts.pop();
            break;
        }
    }
    // [...]
}

As noted in Finding ref↗, this costs a variable amount of gas and could be prohibitively expensive if too many splitters are added.

Since the index of the splitter is known to the caller and more easily computable off chain, this function could take the index as a hint or alternatively just take the index. That will make it run in constant time.

Impact

This requires more gas than necessary.

Recommendations

We recommend either changing removeSplitter to identify the account by index instead of address or adding an additional parameter that serves as a hint for the index. If the latter option is taken, then the function should either revert or do the current loop if the hint is incorrect. This will save gas if there is more than a threshold amount of accounts — the exact threshold depends on the compilation parameters.

Remediation

In addition Stake.link has provided the following response:

We will ensure that the number of splitters will always be small.