Failed stake removals and failed delegate stake removals are not replayable

Summary

Stake removal and delegate stake removal can fail due to several reasons, but there is no mechanism to replay a failing one.

Vulnerability Detail

When a reputer calls RemoveStake at block N, a stake removal request is registered. It will be executed after a delay window (X blocks). The stake removal execution will be automatic at block N+X. (The same applies to RemoveDelegateStake)

The stake removals requests are executed at the end of a block in the RemoveStakes function. This function will execute all stake removals for the current block.

When one of the stake removals fail, this stake removal request is not removed for a potential execution in the future. However, there is no mechanism to execute a stake removal that has failed in a previous block.

Impact

Stake removals and delegate stake removals can be locked after the delay window.

Code Snippet

The EndBlocker function allows executing code out of block. It calls RemoveStakes. See abci.go#L22-L23.

func EndBlocker(ctx context.Context, am AppModule) error {
    // ...

    // Remove Stakers that have been wanting to unstake this block. They no longer get paid rewards
    RemoveStakes(sdkCtx, blockHeight, am.keeper)
    RemoveDelegateStakes(sdkCtx, blockHeight, am.keeper)

    // ...
}

Then, RemoveStakes retrieves all stake removals for the current block. It executes the stake removal in a for loop. If one is failing, this one is not removed.

See stake_removals.go#L13-L63.

// Remove all stakes this block that have been marked for removal
func RemoveStakes(
    sdkCtx sdk.Context,
    currentBlock int64,
    k emissionskeeper.Keeper,
) {
    removals, err := k.GetStakeRemovalsForBlock(sdkCtx, currentBlock) // @POC: Retrieves all stake removals for CURRENT block only
    if err != nil {
        sdkCtx.Logger().Error(fmt.Sprintf(
            "Unable to get stake removals for block %d, skipping removing stakes: %v",
            currentBlock,
            err,
        ))
        return
    }
    for _, stakeRemoval := range removals {
        // do no checking that the stake removal struct is valid. In order to have a stake removal
        // it would have had to be created in msgServer.RemoveStake which would have done
        // validation of validity up front before scheduling the delay

        // Check the module has enough funds to send back to the sender
        // Bank module does this for us in module SendCoins / subUnlockedCoins so we don't need to check // @POC: The module may not have enough funds as shows the comment
        // Send the funds
        coins := sdk.NewCoins(sdk.NewCoin(chainParams.DefaultBondDenom, stakeRemoval.Amount))
        err = k.SendCoinsFromModuleToAccount(sdkCtx, emissionstypes.AlloraStakingAccountName, stakeRemoval.Reputer, coins)
        if err != nil {
            sdkCtx.Logger().Error(fmt.Sprintf(
                "Error removing stake: %v | %v",
                stakeRemoval,
                err,
            ))
            continue // @POC: If the stake removal fails, continue
        }

        // Update the stake data structures
        err = k.RemoveReputerStake( // @POC: Remove the reputer stake removal request if transfer success
            sdkCtx,
            currentBlock,
            stakeRemoval.TopicId,
            stakeRemoval.Reputer,
            stakeRemoval.Amount,
        ) // @POC: WARNING: If the removal fails, tokens are still transfered and the stake removal request is not deleted
        if err != nil {
            sdkCtx.Logger().Error(fmt.Sprintf(
                "Error removing stake: %v | %v",
                stakeRemoval,
                err,
            ))
            continue
        }
    }
}

The same applies to RemoveDelegateStakes which retrieves all delegate stake removals for the current block. If one is failing, this one is not removed.

See stake_removals.go#L66-L118.

Once a stake removal or a delegate stake removal has failed once (for example, due to a lack of balance), the failing one will never be replayable because there is no mechanism to do so.

Tool used

Manual Review

Recommendation

New message entrypoints should be added to execute stake removals and delegate stake removals that failed in a previous block. For example, ExecutePreviousRemoveStake and ExecutePreviousRemoveDelegateStake.

Note: The pattern "transfer tokens then delete stake request" in RemoveStakes and RemoveDelegateStakes is pretty unsafe as if the deletion fails, the tokens are still transfered and the stake removal request is not deleted.

Duplicate of #55

sherlock-admin3 commented 1 month ago

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

0xmystery commented:

Errors in RemoveStakes/RemoveDelegateStakes are silently handled in EndBlocker

sherlock-admin2 commented 1 month ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/allora-network/allora-chain/pull/465

sherlock-audit / 2024-06-allora-judging

zigtur - Failed stake removals and failed delegate stake removals are not replayable #108