Skale Token

Description

It’s possible to create a delegation with a very huge amount which may result in a lot of critically bad malicious usages:

code/contracts/delegation/DelegationRequestManager.sol:L74-L76

uint holderBalance = SkaleToken(contractManager.getContract("SkaleToken")).balanceOf(holder);
uint lockedToDelegate = tokenState.getLockedCount(holder) - tokenState.getPurchasedAmount(holder);
require(holderBalance >= amount + lockedToDelegate, "Delegator hasn't enough tokens to delegate");

amount is passed by a user as a parameter, so if it’s close to uint max value, amount + lockedToDelegate would overflow and this requirement would pass.

Having delegation with an almost infinite amount of tokens can lead to many various attacks on the system up to stealing funds and breaking everything.

Recommendation

Using SafeMath everywhere should prevent this and other similar issues. There should be more critical attacks caused by overflows/underflows, so SafeMath should be used everywhere in the codebase.

Description

Skale token is a modified ERC-777 that allows locking some part of the balance. Locking is checked during every transfer:

code/contracts/ERC777/LockableERC777.sol:L433-L441

// Property of the company SKALE Labs inc.---------------------------------
        uint locked = _getLockedOf(from);
        if (locked > 0) {
            require(_balances[from] >= locked + amount, "Token should be unlocked for transferring");
        }
//-------------------------------------------------------------------------
        _balances[from] = _balances[from].sub(amount);
        _balances[to] = _balances[to].add(amount);

But it’s not checked during burn function and it’s possible to “burn” locked tokens. Tokens will be burned, but locked amount will remain the same. That will result in having more locked tokens than the balance which may have very unpredictable behaviour.

Recommendation

Allow burning only unlocked tokens.

Description

Validators can link a node address to them by calling linkNodeAddress function:

code/contracts/delegation/ValidatorService.sol:L109-L119

function linkNodeAddress(address validatorAddress, address nodeAddress) external allow("DelegationService") {
    uint validatorId = getValidatorId(validatorAddress);
    require(_validatorAddressToId[nodeAddress] == 0, "Validator cannot override node address");
    _validatorAddressToId[nodeAddress] = validatorId;
}

function unlinkNodeAddress(address validatorAddress, address nodeAddress) external allow("DelegationService") {
    uint validatorId = getValidatorId(validatorAddress);
    require(_validatorAddressToId[nodeAddress] == validatorId, "Validator hasn't permissions to unlink node");
    _validatorAddressToId[nodeAddress] = 0;
}

After that, the node has the same rights and is almost indistinguishable from the validator. So the node can even remove validator’s address from _validatorAddressToId list and take over full control over validator. Additionally, the node can even remove itself by calling unlinkNodeAddress, leaving validator with no control at all forever.

Also, even without nodes, a validator can initially call unlinkNodeAddress to remove itself.

Recommendation

Linked nodes (and validator) should not be able to unlink validator’s address from the _validatorAddressToId mapping.

Description

The initial funds can be unlocked if 51+% of them are delegated. However if any portion of the funds are slashed, the rest of the funds will not be unlocked at the end of the delegation period.

code/contracts/delegation/TokenState.sol:L258-L263

if (_isPurchased[delegationId]) {
    address holder = delegation.holder;
    _totalDelegated[holder] += delegation.amount;
    if (_totalDelegated[holder] >= _purchased[holder]) {
        purchasedToUnlocked(holder);
    }

Recommendation

Consider slashed tokens as delegated, or include them in the calculation for process to unlock in endingDelegatedToUnlocked

Description

Bounties are currently locked for the first 3 months after delegation:

code/contracts/delegation/DelegationService.sol:L315

skaleBalances.lockBounty(shares[i].holder, timeHelpers.addMonths(delegationStarted, 3));

Instead, they should be locked for the first 3 months after the token launch.

Recommendation

It’s better just to forbid any withdrawals for the first 3 months, no need to track it separately for every delegation. This recommendation is mainly to simplify the process.

Description

getLockedCount is iterating over all delegations of a specific holder and may even change the state of these delegations by calling getState.

code/contracts/delegation/TokenState.sol:L60-L71

function getLockedCount(address holder) external returns (uint amount) {
    amount = 0;
    DelegationController delegationController = DelegationController(contractManager.getContract("DelegationController"));
    uint[] memory delegationIds = delegationController.getDelegationsByHolder(holder);
    for (uint i = 0; i < delegationIds.length; ++i) {
        uint id = delegationIds[i];
        if (isLocked(getState(id))) {
            amount += delegationController.getDelegation(id).amount;
        }
    }
    return amount + getPurchasedAmount(holder) + this.getSlashedAmount(holder);
}

This problem is major because delegations number is growing over time and may even potentially grow more than the gas limit and lock all tokens forever. getLockedCount is called during every transfer which makes any token transfer much more expensive than it should be.

Recommendation

Remove iterations over a potentially unlimited amount of tokens. All the necessary data can be precalculated before and getLockedCount function can have O(1) complexity.

Description

After the first 3 months since at least 50% of tokens are delegated, all tokens should be unlocked. In practice, they are only unlocked if at least 50% of tokens, that were bought on the initial launch, are undelegated.

code/contracts/delegation/TokenState.sol:L258-L264

if (_isPurchased[delegationId]) {
    address holder = delegation.holder;
    _totalDelegated[holder] += delegation.amount;
    if (_totalDelegated[holder] >= _purchased[holder]) {
        purchasedToUnlocked(holder);
    }
}

Recommendation

Implement lock mechanism according to the legal requirement.

Description

When some amount of tokens are delegated to a validator when the delegation period ends, these tokens are unlocked. However these tokens should be added to _purchased as they were in that state before their delegation.

code/contracts/delegation/TokenState.sol:L258-L264

if (_isPurchased[delegationId]) {
    address holder = delegation.holder;
    _totalDelegated[holder] += delegation.amount;
    if (_totalDelegated[holder] >= _purchased[holder]) {
        purchasedToUnlocked(holder);
    }
}

Recommendation

Tokens should only be unlocked if the main legal requirement (_totalDelegated[holder] >= _purchased[holder]) is satisfied, which in the above case this has not happened.

Description

When some amount of tokens are requested to be delegated to a validator, the validator can reject the request. The previous status of these tokens should be intact and not changed (locked or unlocked).

Here the initial status of tokens gets stored and it’s either completely locked or unlocked:

code/contracts/delegation/TokenState.sol:L205-L214

if (_purchased[delegation.holder] > 0) {
    _isPurchased[delegationId] = true;
    if (_purchased[delegation.holder] > delegation.amount) {
        _purchased[delegation.holder] -= delegation.amount;
    } else {
        _purchased[delegation.holder] = 0;
    }
} else {
    _isPurchased[delegationId] = false;
}

The problem is that if some amount of these tokens are locked at the time of the request and the rest tokens are unlocked, they will all be considered as locked after the delegation was rejected.

code/contracts/delegation/TokenState.sol:L272-L278

function _cancel(uint delegationId, DelegationController.Delegation memory delegation) internal returns (State state) {
    if (_isPurchased[delegationId]) {
        state = purchasedProposedToPurchased(delegationId, delegation);
    } else {
        state = proposedToUnlocked(delegationId);
    }
}

Recommendation

Don’t change the status of the rejected tokens.

Description

After every bounty payment (should be once per month) to a validator, the bounty is distributed to all delegators. In order to do that, there is a for loop that iterates over all active delegators and sends their bounty to SkaleBalances contract:

code/contracts/delegation/DelegationService.sol:L310-L316

for (uint i = 0; i < shares.length; ++i) {
    skaleToken.send(address(skaleBalances), shares[i].amount, abi.encode(shares[i].holder));

    uint created = delegationController.getDelegation(shares[i].delegationId).created;
    uint delegationStarted = timeHelpers.getNextMonthStartFromDate(created);
    skaleBalances.lockBounty(shares[i].holder, timeHelpers.addMonths(delegationStarted, 3));
}

There are also few more loops over all the active delegators. This leads to a huge gas cost of distribution mechanism. A number of active delegators that can be processed before hitting the gas limit is limited and not big enough.

The same issue is with slashing:

code/contracts/delegation/DelegationService.sol:L95-L106

function slash(uint validatorId, uint amount) external allow("SkaleDKG") {
    ValidatorService validatorService = ValidatorService(contractManager.getContract("ValidatorService"));
    require(validatorService.validatorExists(validatorId), "Validator does not exist");

    Distributor distributor = Distributor(contractManager.getContract("Distributor"));
    TokenState tokenState = TokenState(contractManager.getContract("TokenState"));

    Distributor.Share[] memory shares = distributor.distributePenalties(validatorId, amount);
    for (uint i = 0; i < shares.length; ++i) {
        tokenState.slash(shares[i].delegationId, shares[i].amount);
    }
}

Recommendation

The best solution would require major changes to the codebase, but would eventually make it simpler and safer. Instead of distributing and centrally calculating bounty for each delegator during one call it’s better to just store all the necessary values, so delegator would be able to calculate the bounty on withdrawal. Amongst the necessary values, there should be history of total delegated amounts per validator during each bounty payment and history of all delegations with durations of their active state.

Description

ERC-777 token comes with callback functions to the receiver and the sender on every token transfer. This gives re-entrancy opportunities for everyone who’s using this token. There is a chance that other systems might not handle ERC-777 correctly.

Examples

Uniswap reentrancy critical bug: https://medium.com/consensys-diligence/uniswap-audit-b90335ac007

Recommendation

Use ERC-20 standard or remove callback function calls.

Remove callback function usage from the system and replace them with a standard ERC-20 flow:

code/contracts/delegation/SkaleBalances.sol:L55-L68

function tokensReceived(
    address operator,
    address from,
    address to,
    uint256 amount,
    bytes calldata userData,
    bytes calldata operatorData
)
    external
    allow("SkaleToken")
{
    address recipient = abi.decode(userData, (address));
    stashBalance(recipient, amount);
}

code/contracts/delegation/DelegationService.sol:L275-L289

function tokensReceived(
    address operator,
    address from,
    address to,
    uint256 amount,
    bytes calldata userData,
    bytes calldata operatorData
)
    external
    allow("SkaleToken")
{
    require(userData.length == 32, "Data length is incorrect");
    uint validatorId = abi.decode(userData, (uint));
    distributeBounty(amount, validatorId);
}

Description

The naming of the functions should reflect their nature, such as functions starting with “get” should be only getters and do not change state. This will result in confusion developments and the implicit state changes might not be noticed.

Other than getters, some other function or variable names are misleading.

Examples

The following functions are a few examples that are named as getters but they change the state.

• getState -> updateState
  • getDelegationsTotal
  • getDelegationsForValidator
  • getDelegationsByHolder

Some other naming that does not reflect the nature of the functionality:

• getPurchasedAmount -> getPurchasedUnlocked
• tokenState.Sold -> lock

Recommendation

For functions that get and update variables use getAndUpdate naming. Similarly use variable names that reflect the nature of the values they store.

Description

If a validator does not get enough funds to run a node (MSR - Minimum staking requirement), all token holders that delegated tokens to the validator cannot switch to a different validator, and might result in funds getting stuck with the nonfunctioning validator for up to 12 months.

Example

code/contracts/delegation/ValidatorService.sol:L166

require((validatorNodes.length + 1) * msr <= delegationsTotal, "Validator has to meet Minimum Staking Requirement");

Recommendation

Allow token holders to withdraw delegation earlier if the validator didn’t get enough funds for running nodes.

Description

The owner of ValidatorService contract can enable and disable validators. The issue is that when a validator is disabled, it still has its delegations, and delegated funds will be locked until the end of their delegation period (up to 12 months).

code/contracts/delegation/ValidatorService.sol:L84-L90

function enableValidator(uint validatorId) external checkValidatorExists(validatorId) onlyOwner {
    trustedValidators[validatorId] = true;
}

function disableValidator(uint validatorId) external checkValidatorExists(validatorId) onlyOwner {
    trustedValidators[validatorId] = false;
}

Recommendation

It might make sense to release all delegations and stop validator’s nodes if it’s not trusted anymore. However, the rationale behind disabling the validators might be different that what we think, in any case there should be a way to handle this scenario, where the validator is disabled but there are funds delegated to it.

Description

A validator can be created with feeRate > 1000 which would mean that the fee rate would be higher than 100%. Severity is not high because that validator will most likely be not whitelisted.

Also, 100%+ fees would still somehow work and not revert because of the absence of SafeMath.

Recommendation

Add sanity check for the input values in registerValidator, and do not allow adding a validator with a fee rate higher than 100%.

Description

getState function is checking and changing the state of a delegation struct. This function is called in many places in the codebase. Every delegation has a lot of different possible states and all of them are changed implicitly during other transactions, which makes it hard to track the logic in the code and make future changes in the code close to impossible without breaking some functionalities.

Recommendation

The general suggestion would be to minimize the number of implicit storage changes. Many states can be either changed explicitly or be calculated without additional storage changes.

As an option, it’s possible to get rid of state storage slot at all. startDate and endDate fields may set the current state:

• initProposed can be called during the creation of the proposal.
• no need to explicitly change states between ACCEPTED and DELEGATED, you can set the start date on acceptance and no further changes are required.
• no need to switch states between DELEGATED and ENDING_DELEGATED, when delegation is set to end, it’s fine to just have end_date storage slot and make assign the date there when undelegate function is called.
• unlocking funds from delegation (or not accepted request) can be explicit.

Also see issue 5.19 for other suggestions regarding getState usage in the code

Description

_endingDelegations is a list of delegations that is created for optimisation purposes. But the only place it’s used is in getPurchasedAmount function, so only a subset of all delegations is going to be updated.

code/contracts/delegation/TokenState.sol:L159-L164

function getPurchasedAmount(address holder) public returns (uint amount) {
    // check if any delegation was ended
    for (uint i = 0; i < _endingDelegations[holder].length; ++i) {
        getState(_endingDelegations[holder][i]);
    }
    return _purchased[holder];

But getPurchasedAmount function is mostly used after iterating over all delegations of the holder.

Recommendation

Remove _endingDelegations and switch to a mechanism that does not require looping through delegations list of potentially unlimited size.

Description

There are many functions that are defined but not implemented. They have a revert with a message as not implemented.

This results in complex code and reduces readability. Here is a some of these functions within the scope of this audit:

• DelegationService.setMinimumStakingRequirement()
• DelegationService.getAllDelegationRequests()
• DelegationService.getDelegationRequestsForValidator()
• DelegationService.listDelegationRequests()
• DelegationService.getDelegationRequestsForValidator() Many more functions in DelegationService.sol

Examples

code/contracts/delegation/DelegationService.sol:L152-L158

function getAllDelegationRequests() external returns(uint[] memory) {
    revert("Not implemented");
}

function getDelegationRequestsForValidator(uint validatorId) external returns (uint[] memory) {
    revert("Not implemented");
}

Recommendation

If these functions are needed for this release, they must be implemented. If they are for future plan, it’s better to remove the extra code in the smart contracts.

Description

tokenState.setState is used to change the state of the token from:

• PROPOSED to ACCEPTED (in accept())
• DELEGATED to ENDING_DELEGATED (in requestUndelegation()

The if/else statement in setState is too complicated and can be simplified, both to optimize gas usage and to increase readability.

Examples

code/contracts/delegation/TokenState.sol:L173-L197

function setState(uint delegationId, State newState) internal {
    TimeHelpers timeHelpers = TimeHelpers(contractManager.getContract("TimeHelpers"));
    DelegationController delegationController = DelegationController(contractManager.getContract("DelegationController"));

    require(newState != State.PROPOSED, "Can't set state to proposed");

    if (newState == State.ACCEPTED) {
        State currentState = getState(delegationId);
        require(currentState == State.PROPOSED, "Can't set state to accepted");

        _state[delegationId] = State.ACCEPTED;
        _timelimit[delegationId] = timeHelpers.getNextMonthStart();
    } else if (newState == State.DELEGATED) {
        revert("Can't set state to delegated");
    } else if (newState == State.ENDING_DELEGATED) {
        require(getState(delegationId) == State.DELEGATED, "Can't set state to ending delegated");
        DelegationController.Delegation memory delegation = delegationController.getDelegation(delegationId);

        _state[delegationId] = State.ENDING_DELEGATED;
        _timelimit[delegationId] = timeHelpers.calculateDelegationEndTime(delegation.created, delegation.delegationPeriod, 3);
        _endingDelegations[delegation.holder].push(delegationId);
    } else {
        revert("Unknown state");
    }
}

Recommendation

Some of the changes that do not change the functionality of the setState function:

• Remove reverts() and add the valid states to the require() at the beginning of the function
• Remove multiple calls to getState()
• Remove final else/revert as this is an internal function and States passed should be valid More optimization can be done which requires further understanding of the system and the state machine.

function setState(uint delegationId, State newState) internal {
        TimeHelpers timeHelpers = TimeHelpers(contractManager.getContract("TimeHelpers"));
        DelegationController delegationController = DelegationController(contractManager.getContract("DelegationController"));

        require(newState != State.PROPOSED || newState != State.DELEGATED, "Invalid state change");
        State currentState = getState(delegationId);

        if (newState == State.ACCEPTED) {
            require(currentState == State.PROPOSED, "Can't set state to accepted");

            _state[delegationId] = State.ACCEPTED;
            _timelimit[delegationId] = timeHelpers.getNextMonthStart();
        } else if (newState == State.ENDING_DELEGATED) {
            require(currentState == State.DELEGATED, "Can't set state to ending delegated");
            DelegationController.Delegation memory delegation = delegationController.getDelegation(delegationId);

            _state[delegationId] = State.ENDING_DELEGATED;
            _timelimit[delegationId] = timeHelpers.calculateDelegationEndTime(delegation.created, delegation.delegationPeriod, 3);
            _endingDelegations[delegation.holder].push(delegationId);
        }
    }

Description

In order to delegate tokens to a validator, the validator should accept the delegation request, however it’s not possible to remove the delegator for the next period.

Recommendation

For consistency, either allow a validator to undelegate delegators for the next period or remove acceptance mechanism if it’s not needed.

Description

Events in Solidity are used to log major state changes in the system, as for tracebility and also trigger UI changes or user notifications. It is a good practice to use events for every value storage change to be able to trace back the system.

Recommendation

emit events whenever a state change happens. As an example slashing does not emit any events and cannot notify a user unless a service is polling the system state regularly.

Description

DelegationService acts as a gateway for every external call. The problem is that it adds extra complexity to the code, which makes it harder to read and add a new code. Also, it costs more gas because of extra calls between contracts.

Recommendation

The same functionality of DelegationService can be added through UI to allow direct calls to each contract. However, as the whole system is modular and upgradable, it is understandable why using one main contract as the point of interaction might make sense as well.

Description

The system is trusted in a way that there are some owners have the power to do major changes in the system. The most powerful is owner of ContractManager which can update any contract in any way. Even though the system is trusted and this is intended behaviour, it’s possible to mitigate this trust a bit.

Recommendation

Add timelock to major admin functions, so people would know about it beforehand (2 weeks before) and would be able to react somehow.

Severity is minor because if owners of SKALE would want to attack the system in that way, tokens would lose the value anyway, and security of SKALE chains would be unreliable. So it’s unclear what can be done even having that knowledge beforehand.

Description

When a user burns tokens, the following code is called:

new_code/contracts/ERC777/LockableERC777.sol:L413-L426

        uint locked = _getAndUpdateLockedAmount(from);
        if (locked > 0) {
            require(_balances[from] >= locked.add(amount), "Token should be unlocked for burning");
        }
//-------------------------------------------------------------------------

        _callTokensToSend(
            operator, from, address(0), amount, data, operatorData
        );

        // Update state variables
        _totalSupply = _totalSupply.sub(amount);
        _balances[from] = _balances[from].sub(amount);

There is a callback function right after the check that there are enough unlocked tokens to burn. In this callback, the user can delegate all the tokens right before burning them without breaking the code flow.

Recommendation

_callTokensToSend should be called before checking for the unlocked amount of tokens, which is better defined as Checks-Effects-Interactions Pattern.

Description

When slashing happens _delegatedToValidator and _effectiveDelegatedToValidator values are reduced.

new_code/contracts/delegation/DelegationController.sol:L349-L355

function confiscate(uint validatorId, uint amount) external {
    uint currentMonth = getCurrentMonth();
    Fraction memory coefficient = reduce(_delegatedToValidator[validatorId], amount, currentMonth);
    reduce(_effectiveDelegatedToValidator[validatorId], coefficient, currentMonth);
    putToSlashingLog(_slashesOfValidator[validatorId], coefficient, currentMonth);
    _slashes.push(SlashingEvent({reducingCoefficient: coefficient, validatorId: validatorId, month: currentMonth}));
}

When holders process slashings, they reduce _delegatedByHolderToValidator, _delegatedByHolder, _effectiveDelegatedByHolderToValidator values.

new_code/contracts/delegation/DelegationController.sol:L892-L904

if (oldValue > 0) {
    reduce(
        _delegatedByHolderToValidator[holder][validatorId],
        _delegatedByHolder[holder],
        _slashes[index].reducingCoefficient,
        month);
    reduce(
        _effectiveDelegatedByHolderToValidator[holder][validatorId],
        _slashes[index].reducingCoefficient,
        month);
    slashingSignals[index.sub(begin)].holder = holder;
    slashingSignals[index.sub(begin)].penalty = oldValue.sub(getAndUpdateDelegatedByHolderToValidator(holder, validatorId, month));
}

Also when holders are undelegating, they are calculating how many tokens from delegations[delegationId].amount were slashed.

new_code/contracts/delegation/DelegationController.sol:L316

uint amountAfterSlashing = calculateDelegationAmountAfterSlashing(delegationId);

All these values should be calculated one from another, but they all will have different rounding errors after slashing. For example, the assumptions that the total sum of all delegations from holder X to validator Y should still be equal to _delegatedByHolderToValidator[X][Y] is not true anymore. The problem is that these assumptions are still used. For example, when undelegating some delegation with delegated amount equals amount(after slashing), the holder will reduce _delegatedByHolderToValidator[X][Y], _delegatedByHolder[X] and _delegatedToValidator[Y] by amount. Since rounding errors of all these values are different that will lead to 2 possible scenarios:

1. If rounding error reduces amount not that much as other values, we can have uint underflow. This is especially dangerous because all calculations are delayed and we will know about underflow and SafeMath revert in the next month or later.
   Developers already made sure that rounding errors are aligned in a correct way, and that the reduced value should always be larger than the subtracted, so there should not be underflow. This solution is very unstable because it’s hard to verify it and keep in mind even during a small code change.

2. If rounding errors make amount smaller then it should be, when other values should be zero (for example, when all the delegations are undelegated), these values will become some very small values. The problem here is that it would be impossible to compare values to zero.

Recommendation

1. Consider not calling revert on these subtractions and make result value be equals to zero if underflow happens.
2. Consider comparing to some small epsilon value instead of zero. Or similar to the previous point, on every subtraction check if the value is smaller then epsilon, and make it zero if it is.

Description

When slashes are processed by a holder, only _delegatedByHolderToValidator and _delegatedByHolder values are reduced. But _effectiveDelegatedByHolderToValidator value remains the same. This value is used to distribute bounties amongst delegators. So slashing will not affect that distribution.

contracts/delegation/DelegationController.sol:L863-L873

uint oldValue = getAndUpdateDelegatedByHolderToValidator(holder, validatorId);
if (oldValue > 0) {
    uint month = _slashes[index].month;
    reduce(
        _delegatedByHolderToValidator[holder][validatorId],
        _delegatedByHolder[holder],
        _slashes[index].reducingCoefficient,
        month);
    slashingSignals[index.sub(begin)].holder = holder;
    slashingSignals[index.sub(begin)].penalty = oldValue.sub(getAndUpdateDelegatedByHolderToValidator(holder, validatorId));
}

Recommendation

Reduce _effectiveDelegatedByHolderToValidator and _effectiveDelegatedToValidator when slashes are processed.

Description

Every user should iterate over each slash (but only once) and process them in order to determine whether this slash impacted his delegations or not.

However, the check is done during almost every action that the user does because it updates the current state of the user’s balance. The downside of this method is that if there are a lot of slashes in the system, every user would be forced to iterate over all of them even if the user is only trading tokens and only calls transfer function.

If the number of slashes is huge, checking them all in one function would impossible due to the block gas limit. It’s possible to call the checking function separately and process slashes in batches. So this attack should not result in system halt and can be mitigated with manual intervention.

Also, there are two separate pipelines for iterating over slashes. One pipeline is for iterating over months to determine amount of slashed tokens in separate delegations. This one can potentially hit gas limit in many-many years. The other one is for modifying aggregated delegation values.

Recommendation

Try to avoid all the unnecessary iterations over a potentially unlimited number of items. Additionally, it’s possible to optimize some calculations:

1. When slashing signals are processed, all of them always have the same holder. There’s no reason for having an array of signals with the same holder (always with predefined length and values will most likely be zero). It seems possible to remove signals functionality and just aggregate the changes for the Punisher.
2. Try merge two pipelines into one.

Description

There are a lot of operations that write some value to the storage (uses SSTORE opcode) without actually changing it.

Examples

In getAndUpdateValue function of DelegationController and TokenLaunchLocker:

new_code/contracts/delegation/DelegationController.sol:L711-L715

for (uint i = sequence.firstUnprocessedMonth; i <= month; ++i) {
    sequence.value = sequence.value.add(sequence.addDiff[i]).sub(sequence.subtractDiff[i]);
    delete sequence.addDiff[i];
    delete sequence.subtractDiff[i];
}

In handleSlash function of Punisher contract amount will be zero in most cases:

new_code/contracts/delegation/Punisher.sol:L66-L68

function handleSlash(address holder, uint amount) external allow("DelegationController") {
    _locked[holder] = _locked[holder].add(amount);
}

Recommendation

Check if the value is the same and don’t write it to the storage in that case.

Description

TimeHelpers.addMonths() implementation is redundant as it can directly use BokkyPooBahsDateTimeLibrary.addMonths() function.

Recommendation

Simply use return BokkyPooBahsDateTimeLibrary.addMonths() on the same function to prevent further code changes, it’s still a good idea to call addMonth through TimeHelpers contract.

Description

Some functions in the codebase are overloaded. That makes code less readable and increases the probability of missing bugs.

For example, there are a lot of reduce function implementations in DelegationController:

new_code/contracts/delegation/DelegationController.sol:L722-L820

function reduce(PartialDifferencesValue storage sequence, uint amount, uint month) internal returns (Fraction memory) {
    require(month.add(1) >= sequence.firstUnprocessedMonth, "Can't reduce value in the past");
    if (sequence.firstUnprocessedMonth == 0) {
        return createFraction(0);
    }
    uint value = getAndUpdateValue(sequence, month);
    if (value == 0) {
        return createFraction(0);
    }

    uint _amount = amount;
    if (value < amount) {
        _amount = value;
    }

    Fraction memory reducingCoefficient = createFraction(value.sub(_amount), value);
    reduce(sequence, reducingCoefficient, month);
    return reducingCoefficient;
}

function reduce(PartialDifferencesValue storage sequence, Fraction memory reducingCoefficient, uint month) internal {
    reduce(
        sequence,
        sequence,
        reducingCoefficient,
        month,
        false);
}

function reduce(
    PartialDifferencesValue storage sequence,
    PartialDifferencesValue storage sumSequence,
    Fraction memory reducingCoefficient,
    uint month) internal
{
    reduce(
        sequence,
        sumSequence,
        reducingCoefficient,
        month,
        true);
}

function reduce(
    PartialDifferencesValue storage sequence,
    PartialDifferencesValue storage sumSequence,
    Fraction memory reducingCoefficient,
    uint month,
    bool hasSumSequence) internal
{
    require(month.add(1) >= sequence.firstUnprocessedMonth, "Can't reduce value in the past");
    if (hasSumSequence) {
        require(month.add(1) >= sumSequence.firstUnprocessedMonth, "Can't reduce value in the past");
    }
    require(reducingCoefficient.numerator <= reducingCoefficient.denominator, "Increasing of values is not implemented");
    if (sequence.firstUnprocessedMonth == 0) {
        return;
    }
    uint value = getAndUpdateValue(sequence, month);
    if (value == 0) {
        return;
    }

    uint newValue = sequence.value.mul(reducingCoefficient.numerator).div(reducingCoefficient.denominator);
    if (hasSumSequence) {
        subtract(sumSequence, sequence.value.sub(newValue), month);
    }
    sequence.value = newValue;

    for (uint i = month.add(1); i <= sequence.lastChangedMonth; ++i) {
        uint newDiff = sequence.subtractDiff[i].mul(reducingCoefficient.numerator).div(reducingCoefficient.denominator);
        if (hasSumSequence) {
            sumSequence.subtractDiff[i] = sumSequence.subtractDiff[i].sub(sequence.subtractDiff[i].sub(newDiff));
        }
        sequence.subtractDiff[i] = newDiff;
    }
}

function reduce(
    PartialDifferences storage sequence,
    Fraction memory reducingCoefficient,
    uint month) internal
{
    require(month.add(1) >= sequence.firstUnprocessedMonth, "Can't reduce value in the past");
    require(reducingCoefficient.numerator <= reducingCoefficient.denominator, "Increasing of values is not implemented");
    if (sequence.firstUnprocessedMonth == 0) {
        return;
    }
    uint value = getAndUpdateValue(sequence, month);
    if (value == 0) {
        return;
    }

    sequence.value[month] = sequence.value[month].mul(reducingCoefficient.numerator).div(reducingCoefficient.denominator);

    for (uint i = month.add(1); i <= sequence.lastChangedMonth; ++i) {
        sequence.subtractDiff[i] = sequence.subtractDiff[i].mul(reducingCoefficient.numerator).div(reducingCoefficient.denominator);
    }
}

Recommendation

Avoid function overloading as a general guideline.