TOB(pox-5): Bond rollover does not remove the staker's shares from the old bond

[anishnaik]

Title

Bond rollover does not remove the staker's shares from the old bond

Severity

Medium

Difficulty

Medium

Type

Data Validation

Target

stackslib/src/chainstate/stacks/boot/pox-5.clar

Description

When a staker rolls over from an ending bond into a later one, register-for-bond adds the staker's shares to the new bond but never removes them from the old bond. The staker can then reclaim the collateral that backed the old bond while its shares remain in place, so the staker continues to earn the old bond's final-cycle reward on collateral that is no longer held by the contract.

Bond reward shares are stored per reward cycle in three maps keyed by (reward-cycle, bond-index, …): total-shares-staked-for-cycle, signer-shares-staked-for-cycle, and staker-shares-staked-for-cycle. Registration writes a staker's sats into every cycle of the bond term through add-staker-to-bond-cycles. Bond N therefore occupies cycles [c_N, c_N+12) and bond N+6 occupies the adjacent range [c_N+12, c_N+24). Every exit path removes a staker's old shares for the current and remaining cycles through remove-staker-from-bond-cycles: unstake-sbtc, update-bond-registration, and announce-l1-early-exit all call it. The rollover path in register-for-bond does not. It only adds the new bond's shares, as shown in Figure 1.1.

(map-set protocol-bond-memberships tx-sender {
    bond-index: bond-index,
    amount-ustx: amount-ustx,
    signer: signer,
    is-l1-lock: (is-ok btc-lockup),
    amount-sats: sats-total,
})
(map-set protocol-bonds-total-staked bond-index
    (+ current-total-staked sats-total)
)
(try! (add-staker-to-bond-cycles tx-sender signer bond-index first-reward-cycle
    BOND_LENGTH_CYCLES sats-total
))

Figure 1.1: The rollover path adds the new bond's shares with no removal of the old bond's shares (stackslib/src/chainstate/stacks/boot/pox-5.clar#L756-L768).

The leftover shares are live rather than cosmetic because of the rollover timing. The verify-bond-rollover-window check requires burn-block-height >= get-bond-l1-unlock-height(N), which is bond-end(N) - reward_cycle/2, and register-for-bond requires burn-block-height < bond-start(N+6) = bond-end(N). The rollover therefore occurs in the second half of bond N's final cycle, before that cycle's reward is computed. During that window is-bond-active-at-height still reports bond N as active, and calculate-bond-rewards pays it over the shares recorded for that cycle, which still include the rolled-over staker, as shown in Figure 1.2.

(total-sats (get-total-shares-staked-for-cycle reward-cycle (some bond-index)))

Figure 1.2: Bond rewards are distributed over the per-cycle shares, which still credit a rolled-over staker (stackslib/src/chainstate/stacks/boot/pox-5.clar#L2109).

The staker can remove the backing collateral in two ways. Rolling into the new bond with a smaller sats amount triggers the refund branch of roll-sbtc, shown in Figure 1.3, which returns the difference and reduces total-sbtc-staked while the old bond's shares retain the larger amount. Alternatively, rolling in with an equal amount and then calling unstake-sbtc withdraws the full collateral; because unstake-sbtc clamps its changes to [clamp(current-cycle, bond-start, bond-end), bond-end) of the new bond, it never touches the old bond's final-cycle shares.

(if (< new-sbtc old-sbtc)
    (let ((delta (- old-sbtc new-sbtc)))
        (try! (as-contract?
            ((with-ft
                'SM3VDXK3WZZSA84XXFKAFAF15NNZX32CTSG82JFQ4.sbtc-token
                "sbtc-token" delta
            ))
            (try! (contract-call?
                'SM3VDXK3WZZSA84XXFKAFAF15NNZX32CTSG82JFQ4.sbtc-token
                transfer delta tx-sender staker none
            ))
        ))
        (var-set total-sbtc-staked
            (- (var-get total-sbtc-staked) delta)
        )
    )

Figure 1.3: The refund branch of roll-sbtc returns collateral while the old bond's shares remain unchanged (stackslib/src/chainstate/stacks/boot/pox-5.clar#L1840-L1855).

This breaks the protocol invariant that total-sbtc-staked equals the sum of custodied sBTC-backed bond shares. For the old bond's final cycle, the sum of custodied shares exceeds total-sbtc-staked by the withdrawn amount. The scope is bounded to that single cycle, because the rollover window only opens in the old bond's last half-cycle and reward computations for earlier cycles have already run.

Exploit Scenario

Alice holds an sBTC-backed position in bond N with S sats of collateral. During bond N's final cycle she calls register-for-bond for bond N+6 with the same S sats, which is a no-op for roll-sbtc and leaves her bond-N shares in place. She then calls unstake-sbtc against the new bond, which refunds her full S sats and sets total-sbtc-staked to zero without altering bond N's final-cycle shares. When calculate-rewards runs for that cycle, bond N is still active, so it distributes the cycle's reward over shares that still credit Alice. Alice claims her share through a signer-manager she controls, collecting the bond's final-cycle reward with no collateral held by the contract and diluting the reward owed to the bond's honest participants.

Recommendations

Short term, in the rollover path of register-for-bond, settle the old bond's staker and signer rewards through the current cycle and call remove-staker-from-bond-cycles for the old (staker, signer, old-bond-index) over [clamp(current-cycle, old-start, old-end), old-end) with the old amount-sats before adding the new bond's shares, mirroring the teardown in update-bond-registration. This prevents the issue because the staker's old shares will no longer earn after the backing collateral is reduced or withdrawn.

Long term, add an invariant test asserting that, after any rollover, get-total-shares-staked-for-cycle for the old bond's remaining cycles drops by the rolled amount and that total-sbtc-staked equals the sum of custodied sBTC-backed bond shares. This prevents the issue because it will detect any exit or rollover path that mutates collateral without symmetrically updating the per-cycle share maps.