Claim can only be created for a `recipient` once

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L129

Vulnerability details

Claim can only be created for a recipient once

The function creating claims, _createClaimUnchecked(), has the hasNoClaim() modifier, that is defined as opposite hasActiveClaim, meaning it reverts if there is an active claim for a user.

It reverts if _claim.startTimestamp > 0.

A claim is inactive if it has been revoked, or if all the claimable amount has been claimed. The issue is that neither revokeClaim() nor withdraw() modify _claim.startTimestamp.

When calling revokeClaim, _claim.isActive is the only member modified, the startTimestamp is not.

This means if a user has had a claim in the past (revoked or not), it is not possible to create another claim for them .

I consider this an issue as it was not mentioned in the docs/Readme, and while it could be a design decision, given the use cases given in the Readme it is questionable. If we take the example of employee token vesting, this would prevent a companys's new employee to have a vesting claim if they were part of that company in the past.

Impact

Medium

Proof Of Concept

• Alice has a claim. She calls withdraw() after _claim.endTimestamp, redeeming the entire vested amount.
• Some time passes, and the admin wishes to create a new claim for Alice.
• The admin calls createClaim(_recipient == Alice,,,,,)
• In hasNoClaim(Alice) reverts because this check does not pass: startTimestamp still has the value it was set to in her first claim.

Tools Used

Manual Analysis

Mitigation

1 - Modify hasNoClaim() so that it actually becomes the opposite of hasActiveClaim:

-        require(_claim.startTimestamp == 0, "CLAIM_ALREADY_EXISTS");
+        require((_claim.startTimestamp == 0) || (!_claim.isActive), "NO_ACTIVE_CLAIM");

2 - Modify withdraw() so that upon the entire vested amount being withdrawn, _claim.isActive is set to false:

```finalVestedAmount
        numTokensReservedForVesting -= amountRemaining;

+       if (usrClaim.amountWithdrawn == finalVestedAmount) usrClaim.isActive = false;

        // After the "books" are set, transfer the tokens
        // Reentrancy note - internal vars have been changed by now
        // Also following Checks-effects-interactions pattern
        tokenAddress.safeTransfer(_msgSender(), amountRemaining);

[0xean]

dupe of #140