Openzeppelin-contracts: [VestedToken] The balance of a holder can be frozen with a stalker-type attack (Already mitigated by v1.0.6)

[izqui]

Zeppelin v1.0.6 already mitigates the issue with solution 3 (see below). A more permanent and elegant solution is needed.

TL;DR: A malicious actor could freeze the balance of a holder (blocking them from transfering their hard-earned tokens) pretty cheaply.

Attack description:

Given how many decimals tokens normally use, the minimum transferable (and therefore grantable) amount is price is almost 0. Given that the logic for checking how many tokens has a holder vested (done in every transfer) involves iterating its grants array, in the case this array is sufficiently large, the transaction can run out of gas before completing, resulting in the holder not being able to transfer tokens.

Attack flow:

1. Attacker acquires x tokens.
2. Attacker transfers victim the smallest amount of tokens with grantVestedTokens(...)
3. Attacker repeats step 2 as many times as needed. Cost per transaction is the gas fee to perform a grant (less than 300k gas) plus the lost tokens to the victim (if tokens has many decimals, this cost is worthless)
4. When victim attempts to transfer his tokens, transaction goes 'Out of gas' while iterating through his multiple grants created by the attacker.

Solutions:

Possible (and complementary) solutions I can think of, in order of personal preference:

1. Only certain trusted whitelisted addresses can do token grants. There is a whitelister address that can give and remove the ability of addresses to create grants. This has been Aragon's approach for our own token.

2. In order to be able to receive grants, a certain holder needs to whitelist itself or whitelist the addresses she wants to be able to receive grants from.

3. Hard-code the maximum amount of grants a certain holder can receive (lower than what would cause an 'Out of gas' under reasonable limits). This way a holder can be stalked and lose the ability to receive more grants, but this way tokens will never be locked and can be transfered to a new account. For Aragon's usecase (we don't expect holders to have much grants, maximum amount is 2 right now) we have hard-coded it on 20 grants, which is way less than what would cause a problem.

4. Enforce a minimum bound on how many tokens have to be transferred in a grant. Doesn't really solve the problem, but could make the attack pretty expensive.

You can check MiniMeIrrevocableVestedToken.sol for the implementation of 1 and 3 in Aragon's token.

This problem was discovered by @jbaylina while auditing Aragon's code.

[maraoz]

Thanks for the responsible report, giving us time to fix it and push a new release :)

[maraoz]

Closing as the problem was more elegantly fixed by moving the vesting logic to a separate contract (TokenVesting)