VTVL contest - AkshaySrivastav's results

The setAdmin() function in AccessProtected.sol can be used to revoke all admins. This could be a feature to completely renounce ownership of the contract after all claims are set or could be a bug in which one admin either intentionally or unintentionally removes all admin (or all other admins except himself).

Line 161 in VTVLVesting._baseVestedAmount() function should not get executed when cliffAmount is 0. In the case of no cliff amount, i.e. where cliffReleaseTimestamp and cliffAmount are both set as 0, the program execution should not enter the if block.

160    if(_referenceTs >= _claim.cliffReleaseTimestamp) {
161        vestAmt += _claim.cliffAmount;
162    }

Solidity pragma versioning should be upgraded to latest available version. Currently the solidity version in contracts is ^0.8.14 which was found to possess some bugs.

Solidity pragma versioning should be exactly same in all contracts. Currently some contracts use ^0.8.14 but some are fixed to 0.8.14.

No need to re-inherit Context contract in VTVLVesting smart contract as Context is already inherited by AccessProtected contract.

Ownable smart contract is unnecessarily imported in AccessProtected.sol while it is never used. Unnecessary imports decreases the readability of smart contract code.

Unnecessary imports are also present in VTVLVesting.sol. The compilation works completely fine with just importing SafeERC20.sol and AccessProtected.sol.

AccessProtected - contract docs do not match implementaion. The implementation only has multiple equal rights admins and no owner field is present while the docs states something else.

7    /** 
8        @title Access Limiter to multiple owner-specified accounts.
9        @dev Exposes the onlyAdmin modifier, which will revert (ADMIN_ACCESS_REQUIRED) if the caller is not the owner nor the admin.
10    */

VariableSupplyERC20Token.constructor() has an empty @dev tag.

VariableSupplyERC20Token contract mentions an incorrect comment

48    // We can't really have burn, because that could make our vesting contract not work.
49    // Example: if the user can burn tokens already assigned to vesting schedules, it could be unable to pay its obligations.

Token can be made burnable in which users can be allowed to burn their own tokens.

Line 159 in VTVLVesting._baseVestedAmount() contains a misleading comment

159        // We don't check here that cliffReleaseTimestamp is after the startTimestamp 
160        if(_referenceTs >= _claim.cliffReleaseTimestamp) {
161            vestAmt += _claim.cliffAmount;
162        }

cliffReleaseTimestamp can never be after startTimestamp as per the require statements of _createClaimUnchecked().

As per the implementation of vesting contract, Line 21 in VTVLVesting.sol should mention greater than or equal instead of just greater than.

21    /// @dev Our balance of the token must always be greater than this amount.

VTVLVesting.ClaimCreated and VTVLVesting.ClaimRevoked events should also log the admin's address so it can be easily queried which admin created and revoked the claim.

In VTVLVesting contract, before revoking a claim the contract should transfer all the pending/partially vested rewards. Otherwise the entire vesting amount will get revoked.

It is at the discretion of protocol development team to decide whether the current behaviour is intended or not.

At Line 82 of VTVLVesting.constructor(), a better check would be to do _tokenAddress.totalSupply(). As this will also ensure that the input address in indeed a token's address and perform the zero address check as well.

82    require(address(_tokenAddress) != address(0), "INVALID_ADDRESS");

The tokenAddress state variable of VTVLVesting should be renamed to token as this variable represents an IERC20 interface rather that just an address. Renaming it to token aligns better with its usage.

17    IERC20 public immutable tokenAddress;

There should be a factory contract for VTVLVesting contract which can keep track of all vesting contracts deployed by different founders. The Factory contract aligns better with the business usecase of VTVL protocol owners.

From the spec, "The core function of VTVL is to allow users to generate and deploy token vesting smart contracts through our platform."

In VariableSupplyERC20Token.mint() function, non-zero input validation check should be done similar to FullPremintERC20Token.constructor().

In all solidity files, license keyword should be mentioned as // SPDX-License-Identifier: UNLICENSED.

All the actors interacting with a VTVLVesting contract need to fully trust all of its admins. Any one of the potentially infinite admins of VTVLVesting contract has the power to (either intentionally or unintentionally):

• revoke claims of all recipients and withdraw all tokens, resulting in a rugpull attack.
• give or take back the admin rights to or from any ethereum address.
• withdraw any other ERC20 token from the vesting contract.

AccessProtected.setAdmin() function should be marked as external.

VariableSupplyERC20Token.mint() - line 41 can be removed, solidity will automatically revert on underflow in the next line.

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;

VariableSupplyERC20Token.mint() - line 37 can be removed, ERC20._mint() already checks the zero address condition.

37    require(account != address(0), "INVALID_ADDRESS");

VTVLVesting - no need to explicitly initialize variable with 0 value, default value for all uints in Solidity is 0. This issue is present at Line 27 and Line 148.

27    uint112 public numTokensReservedForVesting = 0;

148    uint112 vestAmt = 0;

In VTVLVesting.createClaimsBatch(), it will be better to use Claim[] array as input parameter. This way explicit length checking won't be needed.

VTVLVesting.withdrawAdmin() - no need for line 402.

tokenAddress.safeTransfer() will automatically revert on insufficient balance.

402    require(amountRemaining >= _amountRequested, "INSUFFICIENT_BALANCE");

VTVLVesting.withdrawOtherToken() - no need for line 449.

_otherTokenAddress.safeTransfer() will automatically revert on insufficient balance.

449    require(bal > 0, "INSUFFICIENT_BALANCE");

The hasNoClaim modifier in VTVLVesting is only used once so it is better to use the require statement itself in _createClaimUnchecked() function and remove the modifier from the code.