Bozho - `StakingRewardsV2.sol` contract may not work properly on Optimism due to use of `block.number`

[sherlock-admin3]

Bozho

Medium

StakingRewardsV2.sol contract may not work properly on Optimism due to use of block.number

Summary

Summary

The StakingRewardsV2.sol contract uses block.number within its Checkpoint struct, which can cause issues on the Optimism Layer 2 solution due to the differences in block number handling between Ethereum Layer 1 and Optimism.

1. The Checkpoint struct includes the blk property which stores block.number:

struct Checkpoint {
    uint64 ts;
    uint64 blk;
    uint128 value;
}

2. The block.number is used when adding a new checkpoint:

function _addCheckpoint(Checkpoint[] storage checkpoints, uint256 _value) internal {
    uint256 length = checkpoints.length;
    uint256 lastTimestamp;
    unchecked {
        lastTimestamp = length == 0 ? 0 : checkpoints[length - 1].ts;
    }

    if (lastTimestamp != block.timestamp) {
        checkpoints.push(
            Checkpoint({
                ts: uint64(block.timestamp),
                blk: uint64(block.number),
                value: uint128(_value)
            })
        );
    } else {
        unchecked {
            checkpoints[length - 1].value = uint128(_value);
        }
    }
}

• https://github.com/Kwenta/token/blob/main/contracts/StakingRewardsV2.sol#L555-L580

Vulnerability Detail

The use of block.number in the Checkpoint struct and related logic can cause inconsistencies on Optimism, where block.number does not increment in the same way as on Ethereum mainnet. This discrepancy can lead to inaccurate checkpoint data, affecting the contract’s functionality related to staking and reward calculations.

Impact

The impact of this vulnerability includes:

1. Inaccurate checkpoint records, leading to potential discrepancies in user balances and rewards.
2. Unreliable reward calculations and distributions, causing potential financial loss or unfair reward allocations to users.

I think this should be valid medium severity bug because the protocol devs said this in the previous report: "We decided to keep the block.number in the Checkpoint struct in case we need it in the future. We have however used packing to reduce the gas impact of this."

Code Snippet

function _addCheckpoint(Checkpoint[] storage checkpoints, uint256 _value) internal {
    uint256 length = checkpoints.length;
    uint256 lastTimestamp;
    unchecked {
        lastTimestamp = length == 0 ? 0 : checkpoints[length - 1].ts;
    }

    if (lastTimestamp != block.timestamp) {
        checkpoints.push(
            Checkpoint({
                ts: uint64(block.timestamp),
                blk: uint64(block.number),
                value: uint128(_value)
            })
        );
    } else {
        unchecked {
            checkpoints[length - 1].value = uint128(_value);
        }
    }
}

Recommendation

Just remove the use of block.number because the code already store the block.timestamp in the checkpoints mapping.

Root Cause

No response

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

No response

PoC

No response

Mitigation

No response

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

Strapontin commented:

block.number is stored but never used, so no vulnerability detected