VTVL contest - joestakey's results

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/token/VariableSupplyERC20Token.sol#L41

Vulnerability details

Limited supply of VariableSupplyERC20Token can be bypassed to mint an infinite amount of tokens

VariableSupplyERC20Token is defined as

A ERC20 token contract that allows minting at will, with limited or unlimited supply. No burning possible

In the case of a limited supply, it can be set during deployment with the constructor parameter maxSupply_, which is stored in mintableSupply. It is defined as:

the maximum supply. The contract won't allow minting over this amount.

The issue is that mintableSupply is decremented upon minting. And because of this check, mintableSupply can reach 0. Further mints will bypass the check, meaning it will be possible to mint more tokens that what mintableSupply intended (unlimited supply)

Impact

I consider it Medium because it essentially means a Token supply can be drastically inflated by the admin (with no actual limit), and such dilution would have a direct impact on the token value. The readme does mention the knowledge of admin power, but this issue falls outside of the acknowledge revoking claim powers, because it impacts the token, not the vesting itself.

Proof Of Concept

• Admin deploys VariableSupplyERC20Token with initialSupply_ == 0 and maxSupply_ = 1e6.
• Admin calls mint(address(Vesting), amount == 1e6) to mint the total token supply to the vesting contract. - mintableSupply = 1e6, so the condition line 40 is true, and the call enters the block in the if statement. - the following check passes, as amount == mintableSupply. - then, the following computation is performed: mintableSupply -= amount, meaning mintableSupply is set to 0.
• The maximum supply has been minted
• Admin decides to call mint(address(Vesting), amount == 1e8). - mintableSupply = 0, so the condition line 40 is false. The call bypasses the if block and calls _mint(Vesting, 1e8), minting the tokens.
• The token supply is now 1e8, while its maximum supply was supposed to be 1e6

Tools Used

Manual Analysis

Mitigation

1 - You can use an additional variable to prevent minting from happening if the maxSupply has been minted

• Add a boolean canMint, initialized to true
• In mint(), add the following:

36:     function mint(address account, uint256 amount) public onlyAdmin {
37:         require(account != address(0), "INVALID_ADDRESS");
+           require(canMint, "MAX_SUPPLY ALREADY MINTED")
38:         // If we're using maxSupply, we need to make sure we respect it
39:         // mintableSupply = 0 means mint at will
40:         if(mintableSupply > 0) {
41:             require(amount <= mintableSupply, "INVALID_AMOUNT");
42:             // We need to reduce the amount only if we're using the limit, if not just leave it be
43:             mintableSupply -= amount;
+               // if all the mintable supply has been minted, set canMint to false
+               if (mintableSupply == 0) canMint = false;
44:         }
45:         _mint(account, amount);
46:     }

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);

QA Report

Table of Contents

Summary

Low

• Inconsistent comments for claims.linearVestAmount
• Logic issue if Vesting.tokenAddress is a fee-on-transfer token

Summary

The code quality is very high. The main issue is an inconsistency with the meaning of a variable in different comments.

Inconsistent comments for claims.linearVestAmount

In VTVLVesting.sol, the definition of the struct Claim states that linearVestAmount is the total entitlement. But in createClaim ( and associated internal functions), linearVestAmount is described as:

The total amount to be linearly vested between _startTimestamp and _endTimestamp

meaning the total entitlement is actually:

allocatedAmount = _cliffAmount + _linearVestAmount

Impact

Low

Tools Used

Manual Analysis

Mitigation

Change the comment in the struct definition:

+        uint112 linearVestAmount; // The total amount to be linearly vested between _startTimestamp and _endTimestamp
-        uint112 linearVestAmount; // total entitlement

Logic issue if Vesting.tokenAddress is a fee-on-transfer token

If the token used in the vesting contract is a fee-on-transfer token, it affects the expected behavior of the withdrawals functions:

• A user calling withdraw() will receive less than the amountRemaining that they were expecting to receive.
• The admin calling withdrawAdmin(_amountRequested) will receive less than _amountRequested.

Impact

Low

Tools Used

Manual Analysis

Mitigation

If you decide to accept fee-on-transfer tokens, add comments to explicitly state that in such case the amountRemaining and _amountRequested transferred in both withdrawal functions will not match the amount received by the user