if `removeNominee` is called then `_getSum` will skip a week in its update

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/main/governance/contracts/VoteWeighting.sol#L223-L249 https://github.com/code-423n4/2024-05-olas/blob/main/governance/contracts/VoteWeighting.sol#L586-L636

Vulnerability details

Issue Description

The removeNominee function is used to remove a nominee from the voting system. When the function is called it does invoke both _getWeight on the removed nominee and _getSum to update all other nominees.

The function will also update the pointsSum.bias for the next timestamp nextTime in order to remove the weight associated with nominee being removed, and the function sets the global timestamp timeSum to be nextTime as shown below:

function removeNominee(bytes32 account, uint256 chainId) external {
    // Check for the contract ownership
    if (msg.sender != owner) {
        revert OwnerOnly(owner, msg.sender);
    }

    // Get the nominee struct and hash
    Nominee memory nominee = Nominee(account, chainId);
    bytes32 nomineeHash = keccak256(abi.encode(nominee));

    // Get the nominee id in the nominee set
    uint256 id = mapNomineeIds[nomineeHash];
    if (id == 0) {
        revert NomineeDoesNotExist(account, chainId);
    }

    // Set nominee weight to zero
    uint256 oldWeight = _getWeight(account, chainId);
    uint256 oldSum = _getSum();
    uint256 nextTime = (block.timestamp + WEEK) / WEEK * WEEK;
    pointsWeight[nomineeHash][nextTime].bias = 0;
    timeWeight[nomineeHash] = nextTime;

    // Account for the the sum weight change
    uint256 newSum = oldSum - oldWeight;
    pointsSum[nextTime].bias = newSum;
    timeSum = nextTime;

    ...
}

The fact that the function timeSum equal to nextTime will provoke an issue as nextTime will represent next week timestamp which wasn't yet included in the _getSum function update when its was called, and because now timeSum equals nextTime the _getSum function will skip that week in its calculation as it starts immediatly from the week that will come after (explained by t += WEEK):

function _getSum() internal returns (uint256) {
    // t is always > 0 as it is set in the constructor
    uint256 t = timeSum;
    Point memory pt = pointsSum[t];
    for (uint256 i = 0; i < MAX_NUM_WEEKS; i++) {
        if (t > block.timestamp) {
            break;
        }
        t += WEEK;
        uint256 dBias = pt.slope * WEEK;
        if (pt.bias > dBias) {
            pt.bias -= dBias;
            uint256 dSlope = changesSum[t];
            pt.slope -= dSlope;
        } else {
            pt.bias = 0;
            pt.slope = 0;
        }

        pointsSum[t] = pt;
        if (t > block.timestamp) {
            timeSum = t;
        }
    }
    return pt.bias;
}

This means that the week represented by removeNominee in nextTime call will not get its changes changesSum recorded as the _getSum function will skip that week, and thus the vote weights logic accounting will get compromised which will impact all the staking system

Here is an example showcasing how a timestamp representing a week will be skipped due to the issue in the removeNominee function:

Example Scenario

Assume the current timestamp is block.timestamp = 1650000000 (April 15, 2022, 14:20:00 UTC), and WEEK is defined as 604800 seconds (1 week).

Initial State:

• timeSum = 1649472000 (April 8, 2022, 00:00:00 UTC)
• pointsSum = {1649472000: Point { bias: 100, slope: 2 }}
• changesSum = {}

A nominee is being removed with removeNominee function. The next week's timestamp (nextTime) will be calculated as follows:

• nextTime = (1650000000 + 604800) / 604800 * 604800 = 1650076800 (April 22, 2022, 00:00:00 UTC)

Steps in removeNominee:

1. Assume _getWeight(account, chainId) returns 20.
2. oldWeight = 20
3. _getSum() is called and returns the current sum (100).
4. nextTime = 1650076800
5. pointsWeight[nomineeHash][nextTime].bias = 0 (weight set to zero for removed nominee for next week)
6. timeWeight[nomineeHash] = nextTime (time of weight update for nominee is set to next week)
7. Calculate new sum: newSum = oldSum - oldWeight = 100 - 20 = 80
8. Update sum for next week: pointsSum[nextTime].bias = 80
9. timeSum is set to nextTime: timeSum = 1650076800

State After removeNominee Execution:

• timeSum = 1650076800 (April 22, 2022, 00:00:00 UTC)
• pointsSum = {1649472000: Point { bias: 100, slope: 2 }, 1650076800: Point { bias: 80, slope: 2 }}

Now, when _getSum is called again, it will process weeks starting from timeSum (April 29, 2022), skipping the week (April 22, 2022) because it computes t += WEEK directly and thus the changes for the week April 22, 2022 (any changesSum modifications) were not applied, compromising the voting weight logic and potentially affecting the entire staking system.

Impact

The voting weight logic will be compromised after a nominee get removed.

Tools Used

Manual review, VS Code

Recommended Mitigation

To simplest way to address this issue is to not update timeSum state directly removeNominee and let it be update in when _getSum is called.

Assessed type

Error