VTVL contest - pashov's results

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/69da6e96f94ff3e02b9bb6175e6de2b3e71d3eb0/contracts/token/VariableSupplyERC20Token.sol#L36

Vulnerability details

Proof of concept

The constructor of VariableSupplyERC20Token takes in a maxSupply_ argument, described in the NatSpec as

@param maxSupply_ - What's the maximum supply. The contract won't allow minting over this amount. Set to 0 for no limit.

Also in the mint() method we have the comment

// If we're using maxSupply, we need to make sure we respect it

The issue is that this maxSupply_ can be bypassed easily.

Steps:

1. Let’s say the maximum for our tokens is 5_000_00 * 1e18 - we pass this as the maxSupply_ argument value in the constructor
2. Now mintableSupply holds this value
3. When minting, the code checks if mintableSupply is more than zero, and if it is this means we have a “maxSupply” set
4. Now we can just mint the maximum mintableSupply by passing the same value as amount in mint() so after

mintableSupply -= amount;

now mintableSupply is zero

1. Now next time the admin calls mint() the check from step 3. is bypassed because mintableSupply is zero and the admin can mint tokens endlessly

Impact

If the token admin is compromised or malicious he can mint endless amounts of tokens and dump them on an exchange, basically making the other tokens worthless. This will result in big value losses in all token holders and vesting participants.

Recommendation

Save maxSupply in a storage variable and check totalSupply against it when calling mint()

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/69da6e96f94ff3e02b9bb6175e6de2b3e71d3eb0/contracts/VTVLVesting.sol#L174

Vulnerability details

Proof of concept

In the _baseVestedAmount of VTVLVesting.sol we see the following code

uint40 finalVestingDurationSecs = _claim.endTimestamp - _claim.startTimestamp; // length of the interval
uint112 linearVestAmount = _claim.linearVestAmount * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs;

Let’s look at truncatedCurrentVestingDurationSecs as just the duration passed from the start of the vesting period for the PoC (this doesn’t omit important data in this context).

Now think of the following scenario:

We have a token $TKN that has 6 decimals (those are the decimals of both USDT & USDC). We want to distribute 10,000 of those tokens to a user vested over a 10 year period.

10 years in seconds is 315360000 ****(this is finalVestingDurationSecs)

This means that we will distribute 10,000 * 10^6 = 10 000 000 000 fractions of a token for 315360000 seconds, meaning we will distribute 310 fractions of a token each second - this is linearVestAmount

Now, since finalVestingDurationSecs is so big (315360000) it will almost always round linearVestAmount to zero when dividing by it, up until

_claim.linearVestAmount * truncatedCurrentVestingDurationSecs becomes a bigger number than 315360000, but since _claim.linearVestAmount is 310 we will need the current vesting duration to be at least 1 017 290 seconds which is 12 days postponed vesting. 12 days in a moving market can make a big difference if the user was expecting the tokens to start vesting from the first day.

Impact

Unexpected postponing of vesting can result in waiting times for users to receive their must-be-vested tokens. This period can be used by other token holders to dump the token and decrease the price of it, resulting in a loss of capital for the vesting receiver.

Recommendation

Enforce the contract to work with only 18 decimal tokens with a require statement in the constructor.

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/69da6e96f94ff3e02b9bb6175e6de2b3e71d3eb0/contracts/VTVLVesting.sol#L242

Vulnerability details

Proof of concept

The function _createClaimUnchecked() has input data validation, but it is missing one for endTimestamp apart from checking that it is later than startTimestamp. In the code we see the following comment

// Do we need to check whether _startTimestamp is greater than the current block.timestamp? 
// Or do we allow schedules that started in the past? 
// -> Conclusion: we want to allow this, for founders that might have forgotten to add some users, or to avoid issues with transactions not going through because of discoordination between block.timestamp and sender's local time

This is fine, but we should check that endTimestamp is after block.timestamp - if not, the funds will be immediately vested, since the time has passed. This can be the case if the contract owner puts wrong startTimestamp and endTimestamp for example due to a front-end bug or just wrong input.

Impact

This issue can result in funds immediately vested to user(employee), which is not a desired outcome of this contract. If the user gets the tokens immediately he can just go and dump them on the market, resulting in price crash which is loss of funds for every holder.

Recommendation

Add the following check

require(_endTimestamp > block.timestamp, "INVALID_END_TIMESTAMP");