VTVL contest - RustyRabbit's results

Lines of code

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

Vulnerability details

Impact

Whenever an admin revokes a claim and the recipient has any unclaimed but vested balance , the unclaimed part is also revoked. Take for instance a total amount of 365 tokens vested over 1 year with a release interval of 1 day. If the recipient at day 30 withdraws their due balance they will receive 30 tokens. If then at day 40 the admin revokes the claim the 10 unclaimed tokens are also revoked and the recipient cannot withdraw them anymore where under normal vesting rules that balance is rightfully theirs.

Proof of Concept

In the function revokeClaim() the amount to reclaim by the admin is calculated as the finalVestAmt - _claim.amountWithdrawn. This should be finalVestAmt - vestedAmount(_recipient, uint40(block.timestamp).

Also note that this means a user should be able to withdraw the remaining part after the claim has been revoked which currently is impossible due to the hasActiveClaim() modifier on the withdraw() function which will cause a revert because the claim's isActive is set to false.

Tools Used

NA

Recommended Mitigation Steps

Replace

function revokeClaim(address _recipient) external onlyAdmin hasActiveClaim(_recipient) {
        ....
        uint112 amountRemaining = finalVestAmt - _claim.amountWithdrawn;
        ....

by

function revokeClaim(address _recipient) external onlyAdmin hasActiveClaim(_recipient) {
        ...
        uint112 amountRemaining = finalVestAmt - vestedAmount(_recipient, uint40(block.timestamp);
        ...

and change the withdraw() function to allow a recipient to withdraw the remaining amount due.

Lines of code

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

Vulnerability details

Impact

The hasNoClaim() modifier on the _createClaimUnchecked reverts if the recipient has ever had a claim which has been canceled for whatever reason or the claim has finished (ie. endTimestamp has passed and the balance has been withdrawn).

This means a recipient can only once be given a claim and not reissued one after expiration or revocation. This might not be a huge problem for an EOA (although it also needs to be funded with ETH), but for SC wallets like Gnosis Safe it is as a new Safe would need to be created.

Proof of Concept

The hasNoClaim() modifier reverts here when the claim's startTimestamp is not 0. The revokeClaim() function does not remove the claim from the claims mapping or set the startTimeStamp to 0 and neither does the withdraw() function.

Tools Used

NA

Recommended Mitigation Steps

Incorporate the removal of finished claims from the claims mapping in the withdraw() function (only on the last withdrawal) and revokeClaim() function (only if there is no remaining balance)

cliffReleaseTimestamp as part of the Claim struct is unnecessary.

When a claim is created the cliffReleaseTimestamp is required to be <= startTimestamp.

As the claim isn't valid until after the startTimestamp this means they are in fact equal except for the case where cliffReleaseTimestamp is 0 which is used to indicate there is no cliff release, but this can also be done with the cliffAmount.

As a result the cliffReleaseTimestamp can be removed from the struct and the code simplified. Unless of course a cliff release after the start timestamp is a valid use case (in which case this is should be a higher risk rating), but I presume this is not the case as the comment explicitly affirms this.