MixinGrantKeys:grantKeys possible DoS with (Unexpected) revert

[code423n4]

Handle

GiveMeTestEther

Vulnerability details

Impact / POC

A user can share the minimum possible time of his key (and buy additional time by extending the key to have enough time to make the attack work) to a recipient of the "grantKeys()" such that for this recipient the "require(expirationTimestamp > toKey.expirationTimestamp, 'ALREADY_OWNS_KEY');" reverts. This will revert the whole transaction and no recipient will receive an airdropped key.

An attacker can optimize for which recipient & key expiration timestamp to maximize gas griefing.

This attack is also feasible, by buying a key for a recipient or transferring his "key" to the recipient (buys for himself and transfers to the recipient) if this allows the "require(expirationTimestamp > toKey.expirationTimestamp, 'ALREADY_OWNS_KEY');" to revert.

grantKeys(): https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinGrantKeys.sol#L22

Tools Used

Manual Analysis

Recommended Mitigation Steps

• skip the users that satisfy the require condition (change to an if statement) and don't emit an event for skipped users
• or create a new event for key granting that differentiates by user that received an airdrop an by users that didn't

[0xleastwood]

This sounds like something that should be handled. As the protocol can be impacted by this issue, I'll keep this as medium for now.

[0xleastwood]

Actually, after thinking about this more, the onlyKeyGranterOrManager could simply not include the malicious user in the transaction. To maintain such an attack could also be costly as it requires the attacker to continuously purchase membership. Marking this as low.