VTVL contest - CertoraInc's results

Lines of code

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

Vulnerability details

Impact

The amounts of token are saved in uint112 variables. The _baseVestedAmount, which calculates the amount of token that was vested for the user given a timestamp, contains the following line that calculates the relative amount using the time passed from the start of the linear vesting and the total amount of time that the vesting lasts for:

uint112 linearVestAmount = _claim.linearVestAmount * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs;

_claim.linearVestAmount is a uint112, and truncatedCurrentVestingDurationSecs and finalVestingDurationSecs are uint40s, which means that the calculations will be done for uint112s. This can make the numerator to overflow if the result of the multiplication is big enough, although the final result shouldn't overflow at all.

Proof of Concept

1. 1000 YAM-V2 (which has 24 decimals, so the actual amount is 1e27) are vested to Alice in a time period of an year (52 weeks, i.e. the duration is 31449600).
2. When the year ends, Alice comes to withdraw her 1000 YAM-V2 (assuming she didn't withdraw before).
3. The amount is calculated by 1e27 * 31449600 / 31449600. Because the calculation is done on uint112s, the numerator overflows, as 1e27 * 31449600 > 2 ** 112 - 1. The transaction reverts, Alice can't withdraw her funds, and their are locked in the contract.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider doing the calculations in uint256 variables, and then (safe) casting the results to the wanted variable sizes.

Lines of code

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

Vulnerability details

Impact

The admin can accidentally create the wrong claim for a user and then revoke it, or he would like to renew a vesting for a user which his vesting was ended. However, this is not possible, because the current claim's fields are not zeroed when it is revoked or ended.

Basically every user can have only one claim.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider deleting the claim's details once it is ended or revoked to allow another claim to be created for the user.

Lines of code

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

Vulnerability details

Impact

Some tokens take a transfer fee (e.g. STA, PAXG). Tokens like these will be supported because the vested amount will be the amount that was actually transferred (i.e. the balance of the contract after the transfer), but it will cause the user to receive less than the amount he was supposed to receive.

Proof of Concept

Let's assume the VTVLVesting contract is deployed with the STA token, which takes a 1% transfer fee.

1. The admin transfers 101.01 STA ( = 100 / 0.99, so after the transferred amount will be 100 STA) to the contract so it's balance after the transfer is 100 STA.
2. The admin creates a claim for Alice using 100 STA.
3. The linear vesting end timestamp is over and Alice calls the claim function, but she receives only 1% of it, which is 99 STA. This is the amount she was supposed to get after 99% of her linear vesting time has passed.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider support fee on transfer tokens in a special way that allows transfer of more tokens to match the amount, or write it down so users will know to pre-calculate the amounts from the beginning

Lines of code

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

Vulnerability details

Impact

Two address tokens exists in the blockchain. For example, Synthetix's ProxyERC20 contract is such a token which exists in many forms (sUSD, sBTC...). Tokens as such can be vested, but the admin can withdraw them even if they are vested by providing the other address to the withdrawOtherToken function. The only check in this function is that _otherTokenAddress != tokenAddress, which is irrelevant in the case of two address tokens.

This can make the admin be able to withdraw the vested funds and break the system, because the balance of the contract can be less than the vested amount.

Proof of Concept

1. The VTVLVesting is deployed with the sUSD contract, using its main (proxy) address - 0x57Ab1ec28D129707052df4dF418D58a2D46d5f51.
2. A claim is created for Alice, vesting 1000 sUSD in linear vesting. Assuming this is the only claim currently, the balance of the contract is 1000 sUSD and the value of numTokensReservedForVesting is 1000e18.
3. The admin calls the withdrawOtherToken for 1000e18 sUSD, providing its second address - 0x57Ab1ec28D129707052df4dF418D58a2D46d5f51. The value of numTokensReservedForVesting is still 1000e18, but the balance of the contract is now 0 sUSD.
4. Alice waits for her vest to end, calls the withdraw function, but the function reverts on the call to safeTransfer() because there is insufficient balance of sUSD. Alice can't receive her funds.

Tools Used

Manual audit

Recommended Mitigation Steps

Replace the address check with a balance check - record the vesting token balance of the contract before and after the transfer and assert that they are equal.

QA Report

• A TODO comment is left on line 266 - // Potential TODO: sanity check, if _linearVestAmount == 0, should we perhaps force that start and end ts are the same?
• ETH can get locked in the contract - consider adding a function for the owners to withdraw ETH. ETH can be in the contract pre-deployment or be transferred through self destruct or block rewards.
• uint112 used for amounts can limit amount of tokens that can be vested. There are token with high number of decimals. The maximum amount of tokens that can be vested if the token has 18 decimals is 5192296858534828. If the token has 24 decimals, the maximum amount is 5192296858.534828. This is even more limited because it needs to be multiplied by the duration of the vest and fit in uint112, which can lead to overflow, so the actual amount is much smaller.
• The admin can (by accident or not) create a claim for address(this), which is the address of the contract. This claim won't be withdraw-able and the funds will be locked in the contract.

Gas Optimizations

• Cache the return value of _msgSender() instead of calling it multiple times. This issue can be seen in multiple functions:
  • AccessProtected.constructor()
  • VTVLVesting.withdraw()
  • VTVLVesting.withdrawAdmin()
• Re-arrange the order of the Claim struct's members to reduce its storage size. Currently, the struct takes 3 storage slots:

  struct Claim {
      // First slot - 160 bits
      uint40 startTimestamp
      uint40 endTimestamp;
      uint40 cliffReleaseTimestamp;
      uint40 releaseIntervalSecs;
          
      // Second slot - 224 bits
      uint112 linearVestAmount
      uint112 cliffAmount
  
      // Third slot - 120 bits
      uint112 amountWithdrawn;
      bool isActive;
      }

  However, if the members will be in this order, and the releaseIntervalSecs (which will fit in less than 40 bits) will be saved as a uint32, it will take only 2 storage slots (with even 32 total free bits):

  struct Claim {
      // First slot - 240 bits
      uint112 linearVestAmount
      uint40 startTimestamp
      uint40 endTimestamp;
      uint40 cliffReleaseTimestamp;
      bool isActive;
      
      // Second slot - 256 bits
      uint112 cliffAmount
      uint112 amountWithdrawn;
      uint32 releaseIntervalSecs;
      }

  Note: additional 16 bits can be saved in the first slot, so if the size of releaseIntervalSecs is not enough we can add more information for it in the first slot, but it is supposed to be enough (maximum interval of 2 ** 32 - 1 ).

• Don't initialize variables to their default value - it is done automatically for you, and it actually costs more if you do it yourself. This can be seen in:
  • VTVLVesting line 27
  • VTVLVesting line 148
  • VTVLVesting line 353
• The _baseVestedAmount checks first if _referenceTs >= _claim.cliffReleaseTimestamp, and then if _referenceTs > _claim.startTimestamp. But we know that _claim.cliffReleaseTimestamp <= _claim.startTimestamp, so we don't need to check the first condition if the second is met. The code should look like this:

      // Calculate the linearly vested amount - this is relevant only if we're past the schedule start
      // at _referenceTs == _claim.startTimestamp, the period proportion will be 0 so we don't need to start the calc
      if(_referenceTs > _claim.startTimestamp) {
          vestAmt += _claim.cliffAmount;
  
          uint40 currentVestingDurationSecs = _referenceTs - _claim.startTimestamp; // How long since the start
          // Next, we need to calculated the duration truncated to nearest releaseIntervalSecs
  
          uint40 truncatedCurrentVestingDurationSecs = (currentVestingDurationSecs / _claim.releaseIntervalSecs) * _claim.releaseIntervalSecs;
          uint40 finalVestingDurationSecs = _claim.endTimestamp - _claim.startTimestamp; // length of the interval
  
          // Calculate the linear vested amount - fraction_of_interval_completed * linearVestedAmount
          // Since fraction_of_interval_completed is truncatedCurrentVestingDurationSecs / finalVestingDurationSecs, the formula becomes
          // truncatedCurrentVestingDurationSecs / finalVestingDurationSecs * linearVestAmount, so we can rewrite as below to avoid 
          // rounding errors
          uint112 linearVestAmount = _claim.linearVestAmount * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs;
  
          // Having calculated the linearVestAmount, simply add it to the vested amount
          vestAmt += linearVestAmount;
      } else if (_referenceTs >= _claim.cliffReleaseTimestamp) {
          vestAmt += _claim.cliffAmount;
      }

• Add the unchecked modifier on calculations that we know that won't underflow or overflow. This can be seen in:
  • VTVLVesting line 167
  • VTVLVesting line 170
  • VTVLVesting line 264
  • VTVLVesting line 353 (loop index incrementation)
  • VTVLVesting line 377
  • VTVLVesting line 429
  • VariableSupplyERC20Token line 43
• Use currentVestingDurationSecs - currentVestingDurationSecs % _claim.releaseIntervalSecs instead of (currentVestingDurationSecs / _claim.releaseIntervalSecs) * _claim.releaseIntervalSecs
• Save the vesting duration as a member of the Claim struct instead of calculating it every time (_claim.endTimestamp - _claim.startTimestamp) in the _baseVestedAmount function
• Use calldata instead of memory when passing struct or array arguments to functions. This can be seen in the createClaimsBatch function.
• Use memory instead of storage when fetching the Claim struct of a user inside a function to avoid multiple SLOADs to get the different values of the struct. This can be seen at:
  • line 106
  • line 420
• Cache the calculation of _linearVestAmount + _cliffAmount in line 256 in the _createClaimUnchecked function. This calculation is done again in the end of this function (line 292), and caching it will save gas.
• Cache the value of numTokensReservedForVesting + allocatedAmount in line 295, and store it later in numTokensReservedForVesting instead of adding allocatedAmount to it again. This will both save gas for the double calculation and will save an SLOAD.
• Use prefix incrementation instead of postfix incrementation for loops, i.e. ++i instead of i++. This will save gas and has the same effect in this case.
• Cache usrClaim.amountWithdrawn in the withdraw and revokeClaim functions to save SLOADs. This value is accessed multiple times, and caching it will reduce the number of SLOADs.
• Implement the constructor of the VariableSupplyERC20Token contract differently to save gas spent on:

1. Double processing of the same expression
2. Multiple checks and storage operation due to calling the mint function
3. Redundant check that the message sender is not the zero address The new code will look like this:

constructor(string memory name_, string memory symbol_, uint256 initialSupply_, uint256 maxSupply_) ERC20(name_, symbol_) {
        // max supply == 0 means mint at will. 
        // initialSupply_ == 0 means nothing preminted
        // Therefore, we have valid scenarios if either of them is 0
        // However, if both are 0 we might have a valid scenario as well - user just wants to create a token but doesn't want to mint anything
        // Should we allow this?
        bool initialSupplyPositive = initialSupply_ > 0;
        require(initialSupplyPositive || maxSupply_ > 0, "INVALID_AMOUNT");
        
        if(initialSupplyPositive) {
            require(initialSupply_ <= maxSupply_, "INVALID_AMOUNT");
            unchecked {
                maxSupply_ -= initialSupply_;
            }
            _mint(_msgSender(), initialSupply_);
        }

        mintableSupply = maxSupply_;
    }

• Use != 0 instead of > 0 when comparing uints to 0. This can be seen in:
  • VTVLVesting line 110
  • VTVLVesting line 268
  • VTVLVesting line 269
  • VTVLVesting line 275
  • VTVLVesting line 286
  • VTVLVesting line 287
  • VTVLVesting line 473
  • FullPremintERC20Token line 11
  • VariableSupplyERC20Token line 27
  • VariableSupplyERC20Token line 31
  • VariableSupplyERC20Token line 40