VTVL contest - rokinot's results

Lines of code

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

Vulnerability details

Impact

Rewards of high value will be unable to be withdrawn since claimableAmount() will revert.

Proof of Concept

In line 176, if we assume there's a claim of a token with 18 decimals and a vesting of 1 year, which is a realistic vesting period that can be observed for deployed and battle tested protocols like Curve, a period that is translated to 31_536_000 seconds, then the operation _claim.linearVestAmount * truncatedCurrentVestingDurationSecs will overflow if there's a claim of at least (2^112) - 1 / (1e18 * 31_536_000) tokens, which is 164 million tokens.

This high vesting amount may be achieved if there's a token allocation for the team/investors, which often can be observed to take 10~30% of the total supply. The problem becomes more exarcebated for tokens with high supplies, as claims will be larger in nominal terms and thus easier to reach the 112 bits cap. For example, a cryptocurrency like JPEG'd which has total supply of 69 billion, would overflow with the equivalent of 91 thousand dollars in tokens (current price as the time of this writing is $0.00055676 USD).

Tools Used

Code reading, Remix

Recommended Mitigation Steps

Change the line to

uint112 linearVestAmount =  truncatedCurrentVestingDurationSecs * (_claim.linearVestAmount / finalVestingDurationSecs);

For tokens with 18 decimals (which is industry standard), the division above will only round down to zero for very small amounts and high vesting periods, if we use our example of one year vesting again, then _claim.linearVestAmount would have to be lower than finalVestingDurationSecs, equivalent to 3.1536e-11 tokens. The variable truncatedCurrentVestingDurationSecs already guarantees there will be no rounding errors.

Low

Contracts can have no administrators, leaving tokens meant for vesting to be stuck forever

An administrator can revoke it's own power calling setAdmin() with it's own address and boolean as false. This will leave unallocated tokens and tokens accidentally sent to the contract stuck forever, since it will be impossible to set a new administrator.

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/AccessProtected.sol#L39-L43

Consider reverting the setAdmin transaction if there's only one administrator set. It's worthy noting this may also be considered a feature rather than a bug.

require statements with and conditionals should be separated into different require statements, consuming less gas.

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L344-L351

Consider using custom errors, as they consume less gas than require() statements.

There are 17 instances in VTVLVesting.sol and 2 in AccessProtected.sol