Unbonding validator random selection can be predicted

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-anchor/blob/7af353e3234837979a19ddc8093dc9ad3c63ab6b/contracts%2Fanchor-bAsset-contracts%2Fcontracts%2Fanchor_basset_hub%2Fsrc%2Funbond.rs#L344

Vulnerability details

Impact

When unbonding, the pick_validator function is supposed to choose a random validator to unstake from. However, this randomness can be predicted knowing the block height which is very easy to predict.

let mut iteration_index = 0;

while claimed.u128() > 0 {
    let mut rng = XorShiftRng::seed_from_u64(block_height + iteration_index);
    let random_index = rng.gen_range(0, deletable_delegations.len());
    // ...
}

As validators are paid rewards proportional to their stake, choosing where to bond and unbond from directly impacts validator rewards. Combined with being able to choose the validator when bonding (see execute_bond) a validator can steal rewards and increase their own.

POC

1. Validator A wishes to increase their rewards. They call execute_bond(, validator = A) with their validator.
2. Validator A precomputes the randomness for unbonding if the block was mined at the next block height. If it's a different validator, they call unbond. The stake is removed from some other validator and the user receives it back. Note that validator A's stake did not decrease and contains the initial bond amount.
3. They repeat this process until every single bond of the Anchor platform is now staked to validator A and they earn huge rewards while everyone else does not earn anything from Anchor bonds anymore.

Recommended Mitigation Steps

An unbond must always remove stake from the validator the stake was originally bonded to, otherwise, it's exploitable by the attack above. Consider keeping track of all bonds with a bond_id and create a map of bond_id -> validator. If a validator's stake decreased due to slashing, take the remaining unstake amount proportionally from all other validators.

[GalloDaSballo]

Looks like weak RNG, Med or High seem proper

[albertchon]

Indeed, block height should definitely not be used as a source of randomness.