Telcoin: tel-v3-staking

Description

claimable() and claimableAt() sum an account's payout over a list of epochIds decoded from auxData, without checking that the ids are unique. Being view functions, they never write the _claimedAtBlock ledger, so a repeated id passes the _claimedAtBlock[...] != 0 guard on every iteration and its _grossPayout(...) is added again. For example, a caller passing [5, 5, 5] receives three times epoch 5's payout.

While no in-scope contract currently consumes these functions, if a downstream contract that treats the returned total as authoritative is introduced in the future, it could act on an arbitrarily inflated amount.

Recommendation

Consider either documenting that the total is only valid for a duplicate-free list, or enforcing uniqueness by requiring the caller to pass epochIds pre-sorted in strictly increasing order. The latter approach can be validated with the following snippet inside the for loop:

if (i != 0 && epochId <= prev) revert UnsortedEpochIds(epochId, prev);prev = epochId;

Description

The deactivation window exists to give users time to claim outstanding rewards before a plugin goes dark, as stated in the startDeactivation() docs. This protection can be bypassed by the owner. In both SimplePlugin and StreamingRewardsPlugin, claim() and increaseClaimableBy() are gated by whenNotPaused, but _deactivated() advances purely on block.timestamp and cleanupPostDeactivation() is gated only by whenDeactivated, not whenNotPaused.

As a result, the owner can call startDeactivation() and then pause(). The deactivation timer keeps running while the contract is paused, so claim() reverts for the entire window. Once the window closes, the owner calls cleanupPostDeactivation(), which still executes while paused and sweeps all remaining reward funds to the owner. Users with outstanding rewards receive nothing.

The ability to pause is a legitimate response to a security incident, so this may be an acceptable tradeoff. It is worth being aware of, as the protection the deactivation window is meant to provide ultimately depends on the trust level of the plugin owner.

Recommendation

Consider freezing the deactivation timer while the contract is paused.

Description

The single-credit increaseClaimableBy() has no account != address(0) check, while the batch path increaseClaimableByBatch() rejects a zero address with ZeroAddress. Calling increaseClaimableBy(address(0), amount) pulls amount reward tokens into the plugin, credits _claimable[address(0)], and adds amount to _totalOwed.

StakingModule always claims on behalf of msg.sender, so address(0) can never claim, and the credited funds are permanently locked. totalClaimable() stays overstated by amount, and because that amount is counted in _totalOwed it is treated as owed rather than free, so rescueTokens() cannot recover it either.

Recommendation

Consider adding the same guard used by the batch path:

+if (account == address(0)) revert ZeroAddress();

Description

Streaming rewards pay rewardAmount * getPastVotes(account, refBlock) / totalSupplyAtRef, a single-block snapshot taken at refBlock. An account's reward is proportional to its stake at one block per cycle, with no weighting by how long the stake was actually held. The refBlock < block.number rule prevents front-running a snapshot that is already known, but it does not prevent positioning for a predictable future snapshot.

This design raises three concerns:

1. Timing predictability. Epochs are expected to be funded frequently, but the cadence must not be predictable. If refBlock lands at the same time each day, or even within a predictable window, a user can stake only around the anticipated block and unstake afterward, capturing a full proportional share for minimal staking duration.
2. Fairness. Because only the snapshot counts, rewards track when a user happens to be staked at the checkpoint rather than the total time staked. Two stakers holding the same amount are rewarded very differently based purely on timing.
3. Distributor trust. The distributor selects refBlock, so it could time epochs around its own stake to disproportionately reward itself.

The withdrawal delay partially mitigates the first two by keeping capital locked, with effectiveness depending on the delay length relative to epoch frequency, but it does not address the underlying design.

Recommendation

Consider a design that rewards stakers based on the duration staked rather than presence at discrete checkpoints. One approach is to give epochs a start and end time, ideally contiguous, and on each claim or stake/unstake sum the rewards for every full epoch the account was staked through, then pro-rate partially staked epochs by the time staked within them. The same effect can be achieved without epochs by accruing a constant reward rate per unit of stake per unit of time (a reward-per-token accumulator), and the rate itself can be made a function of time if a variable schedule is desired.

If the snapshot design is retained, ensure the epoch cadence and refBlock selection are unpredictable and jittered, and do not run epochs while the withdrawal delay is 0.

Description

migrateBatch(), gated by BATCH_MIGRATOR_ROLE, exits each listed user's V2 stake through claimAndExitFor(), swaps it to V3, and re-stakes it to the user via stakeForBatch(). No approval or signature is required from the users being migrated. Each user receives their equivalent V3 stake, so there is no loss, but a user has no say in whether or when they are migrated.

This matters if some users would prefer not to migrate, for example because their V2 stake is still earning rewards, they want to remain liquid, or they do not want to be subject to the V3 withdrawal delay. The design therefore depends on the BATCH_MIGRATOR_ROLE holder being fully trusted to migrate only users who have authorized it off-chain.

Recommendation

If forced migration is not intended, consider requiring each user to sign a message authorizing their own migration and validating that signature in migrateBatch() before exiting their V2 stake. If forced migration is intended, consider documenting this trust assumption clearly so users understand their stake can be moved without an on-chain action on their part.

Description

migrateAll() is documented to preserve each caller's staked versus unstaked split by re-staking only the previously staked portion and returning the previously unstaked portion to the wallet. However, there is one exception to this pattern.

Leg 1 calls claimAndExitFor(), which pays both the exited staked principal and any plugin rewards claimed through v2AuxData to the migrator. stakedPulled is measured as the aggregate balance delta, so it includes those rewards. Leg 3 then stakes the whole amount as sTEL, so reward tokens that were not staked before migration end up staked. The MigratedAll event's stakedPulled and stakedV3 fields likewise over-report principal. Because the resulting sTEL value is equivalent, no funds are lost. Regardless, the actual behavior differs from the documented behavior.

Recommendation

Consider either measuring the exited principal and the claimed rewards separately, so the principal can be re-staked while the rewards are returned to the wallet as unstaked, or documenting that rewards claimed through v2AuxData are folded into the staked portion.

Description

The StreamingRewardsPlugin constructor declares a parameter named _deactivationDelay, which shares its name with the storage variable inherited from DeactivationTimelock. The constructor uses the locally scoped parameter as intended, so behavior is correct, but the shared name makes it harder to distinguish the parameter from the storage variable.

Recommendation

Consider renaming the parameter, for example to deactivationDelay_, and updating its references in the constructor:

- constructor(address _staking, address _rewardToken, uint256 _deactivationDelay)+ constructor(address _staking, address _rewardToken, uint256 deactivationDelay_)      Ownable(msg.sender)-     DeactivationTimelock(_deactivationDelay)+     DeactivationTimelock(deactivationDelay_)

Description

The _authorizeUpgrade() documentation describes UPGRADER_ROLE as the top of the trust pyramid, protected by a multisig and timelock so any malicious upgrade has a public window during which users can exit. Roles are managed with OpenZeppelin AccessControl, where DEFAULT_ADMIN_ROLE is the admin of every role, including UPGRADER_ROLE and itself.

A DEFAULT_ADMIN_ROLE holder can therefore grant UPGRADER_ROLE to any address, including itself, and revoke or reassign roles at will, which places it above UPGRADER_ROLE. If UPGRADER_ROLE is guarded by a timelock but DEFAULT_ADMIN_ROLE is not, an admin could grant itself UPGRADER_ROLE and upgrade without the intended public window, bypassing the protection.

Recommendation

Consider ensuring DEFAULT_ADMIN_ROLE is protected at least as strongly as UPGRADER_ROLE, for example a multisig combined with a timelock.

Description

The NatSpec block above DeactivationTimelock uses @title TieredOwnership, which names a different contract and appears to have been copied from TieredOwnership.sol. The title does not describe the contract it sits above.

Recommendation

Update the @title tag to match the contract it documents.

Description

updateRewardToken() in FeeBuybackProxySafe changes the telcoin reward token but does not emit an event. The TokenUpdated event already exists and is emitted by initialize() when the token is first set, so the update path is inconsistent and leaves offchain monitoring unaware of any later change.

Recommendation

Consider emitting TokenUpdated after the assignment:

function updateRewardToken(    IERC20 token) public onlyRole(SUPPORT_ROLE) {    telcoin = token;+   emit TokenUpdated(address(telcoin));}

Description

initialize() sets $.withdrawalDelay to initialWithdrawalDelay_ but does not emit an event. The WithdrawalDelaySet event already exists and is emitted by setWithdrawalDelay() on every later change, so the initial value is the only one not surfaced through an event.

Recommendation

Consider emitting WithdrawalDelaySet from the old value of 0 to the initial delay after the assignment:

$.withdrawalDelay = initialWithdrawalDelay_;+emit WithdrawalDelaySet(0, initialWithdrawalDelay_);

Description

_deactivationDelay in DeactivationTimelock is assigned only in the constructor and is never modified afterward. Both SimplePlugin and StreamingRewardsPlugin pass a fixed value into that constructor and are deployed directly rather than behind a proxy, so the value is set once at deployment.

Declaring it immutable stores it in contract bytecode rather than storage, replacing the storage load in _startDeactivation() and the public getter with a cheaper bytecode read.

Recommendation

Consider marking the variable immutable:

- uint256 public _deactivationDelay;+ uint256 public immutable _deactivationDelay;

Description

In the increase branch of setWithdrawalDelay(), the per-day cap is computed as (elapsed * MAX_DELAY_INCREASE_PER_DAY) / 1 days. Since MAX_DELAY_INCREASE_PER_DAY is set to 1 days, this expression is always equal to elapsed, so the multiplication and division are redundant. The compiler does not fold them because it must preserve the overflow behavior of the intermediate elapsed * 1 days.

Recommendation

Consider comparing against elapsed directly:

- uint256 maxIncrease = (elapsed * MAX_DELAY_INCREASE_PER_DAY) / 1 days;  uint256 requested = newDelay - oldDelay;- if (requested > maxIncrease) revert DelayIncreaseTooFast(requested, maxIncrease);+ if (requested > elapsed) revert DelayIncreaseTooFast(requested, elapsed);

Note that this couples the check to MAX_DELAY_INCREASE_PER_DAY being equal to 1 days. If that constant is ever changed, the simplification must be revisited.