VTVL contest - 0xA5DF's results

Lines of code

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

Vulnerability details

From the contest description it seems that the sponsor doesn't intend to allow admin to revoke a claim after it end:

our vesting contract is deliberately designed to allow admin revocation in the circumstances of early employment termination (before the end of vesting period specified).

However, it's still possible for an admin to revoke it as long as the recipient hasn't withdrawn it.

Impact

A recipient will loose the amount of the claim he hasn't withdrawn.

You might assume users will withdraw their funds as soon as they're available and therefore the likelihood of a significant amount of the claim remaining there is low, but I think in reality people often get caught up with other stuff and postpone dealing with their financial stuff (esp. in the web3 world, which might be complicated and time consuming for new users).

Proof of Concept

The following test checks that scenario (passing = bug exists): (The test is put in place of the test at VTVLVesting.ts#L503-L511 )

  it('allows admin to revoke a valid claim', async () => {
    const {vestingContract} = await createPrefundedVestingContract({tokenName, tokenSymbol, initialSupplyTokens});
    let tx = await vestingContract.createClaim(recipientAddress, startTimestamp, endTimestamp, cliffReleaseTimestamp, releaseIntervalSecs, linearVestAmount, cliffAmount);
    let reciept = await tx.wait();
    let block = await ethers.provider.getBlock(reciept.blockNumber);
    

    await ethers.provider.send("evm_increaseTime", [endTimestamp.toNumber() - block.timestamp + 3600]);
    tx = await vestingContract.revokeClaim(recipientAddress);
    reciept = await (tx).wait();
    block = await ethers.provider.getBlock(reciept.blockNumber);

    expect(block.timestamp).is.greaterThan(endTimestamp.toNumber());

    expect(await (await vestingContract.getClaim(recipientAddress)).isActive).to.be.equal(false);
  });

Recommended Mitigation Steps

Require that the claim hasn't ended before revoking it.

Admin might remove himself

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

There's no check that the admin isn't _msgSender(), in a scenario where the sender is only admin that can lead to loss of governance over the protocol.

Impact

Losing governance over the protocol will lead to loss of the ability to:

• Create new claims
• Revoke claims
• Withdrawing any remaining funds (withdrawAdmin())

Meaning, any remaining funds that aren't reserved for existing claims will be frozen.

Mitigation

Require that the _msgSender() != admin

Zero initial supply allowed when max supply is limited but not when it's unlimited

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

There's already a comment in the code addressing this, but I'd just like to point out that there's no reason to require initial supply to be greater than zero only when the max supply is unlimited. Not allowing initial supply to be zero when more tokens can be minted afterwards (unlimited > limited) doesn't make much sense.

Mitigation

Remove the requirement at line 27 (require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");). In anyways the actual max supply is greater than zero.

Cache storage variables

Caching can be done at 2 places:

• _createClaimUnchecked() for numTokensReservedForVesting that is being read twice (the 2nd is implicit at a += assignment)
  • ~100 units saved per call
• withdraw() - the usrClaim variable is used a few times
  • ~600 units saved per call

Code diff:

diff --git a/contracts/VTVLVesting.sol b/contracts/VTVLVesting.sol
index c564c6f..70f51d7 100644
--- a/contracts/VTVLVesting.sol
+++ b/contracts/VTVLVesting.sol
@@ -291,14 +291,15 @@ contract VTVLVesting is Context, AccessProtected {
         // Not necessary to use the more complex logic from _baseVestedAmount
         uint112 allocatedAmount = _cliffAmount + _linearVestAmount;
 
+        uint112 _numTokensReservedForVesting = numTokensReservedForVesting;
         // Still no effects up to this point (and tokenAddress is selected by contract deployer and is immutable), so no reentrancy risk 
-        require(tokenAddress.balanceOf(address(this)) >= numTokensReservedForVesting + allocatedAmount, "INSUFFICIENT_BALANCE");
+        require(tokenAddress.balanceOf(address(this)) >= _numTokensReservedForVesting + allocatedAmount, "INSUFFICIENT_BALANCE");
 
         // Done with checks
 
         // Effects limited to lines below
         claims[_recipient] = _claim; // store the claim
-        numTokensReservedForVesting += allocatedAmount; // track the allocated amount
+        numTokensReservedForVesting = _numTokensReservedForVesting + allocatedAmount; // track the allocated amount
         vestingRecipients.push(_recipient); // add the vesting recipient to the list
         emit ClaimCreated(_recipient, _claim); // let everyone know
     }
@@ -365,20 +366,21 @@ contract VTVLVesting is Context, AccessProtected {
         // Get the message sender claim - if any
 
         Claim storage usrClaim = claims[_msgSender()];
+        Claim memory _usrClaimCached = usrClaim;
 
         // we can use block.timestamp directly here as reference TS, as the function itself will make sure to cap it to endTimestamp
         // Conversion of timestamp to uint40 should be safe since 48 bit allows for a lot of years.
-        uint112 allowance = vestedAmount(_msgSender(), uint40(block.timestamp));
+        uint112 allowance = _baseVestedAmount(_usrClaimCached, uint40(block.timestamp));
 
         // Make sure we didn't already withdraw more that we're allowed.
-        require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW");
+        require(allowance > _usrClaimCached.amountWithdrawn, "NOTHING_TO_WITHDRAW");
 
         // Calculate how much can we withdraw (equivalent to the above inequality)
-        uint112 amountRemaining = allowance - usrClaim.amountWithdrawn;
+        uint112 amountRemaining = allowance - _usrClaimCached.amountWithdrawn;
 
         // "Double-entry bookkeeping"
         // Carry out the withdrawal by noting the withdrawn amount, and by transferring the tokens.
-        usrClaim.amountWithdrawn += amountRemaining;
+        usrClaim.amountWithdrawn = _usrClaimCached.amountWithdrawn + amountRemaining;
         // Reduce the allocated amount since the following transaction pays out so the "debt" gets reduced
         numTokensReservedForVesting -= amountRemaining;
         

Gas report diff (- is before, + is after):

 ···················|······················|··············|·············|·············|···············|··············
 |  TestERC20Token  ·  transfer            ·       47587  ·      52339  ·      47809  ·           45  ·          -  │
 ···················|······················|··············|·············|·············|···············|··············
-|  VTVLVesting     ·  createClaim         ·      139603  ·     173947  ·     167756  ·           28  ·          -  │
+|  VTVLVesting     ·  createClaim         ·      139472  ·     173816  ·     167627  ·           28  ·          -  │
 ···················|······················|··············|·············|·············|···············|··············
-|  VTVLVesting     ·  createClaimsBatch   ·      181558  ·     387422  ·     284486  ·            3  ·          -  │
+|  VTVLVesting     ·  createClaimsBatch   ·      181427  ·     387029  ·     284224  ·            3  ·          -  │
 ···················|······················|··············|·············|·············|···············|··············
 |  VTVLVesting     ·  revokeClaim         ·           -  ·          -  ·      40827  ·            6  ·          -  │
 ···················|······················|··············|·············|·············|···············|··············
 |  VTVLVesting     ·  setAdmin            ·       26511  ·      48423  ·      37467  ·            4  ·          -  │
 ···················|······················|··············|·············|·············|···············|··············
-|  VTVLVesting     ·  withdraw            ·       61171  ·      78271  ·      72938  ·           10  ·          -  │
+|  VTVLVesting     ·  withdraw            ·       60598  ·      77698  ·      72365  ·           10  ·          -  │
 ···················|······················|··············|·············|·············|···············|··············
 |  VTVLVesting     ·  withdrawAdmin       ·       43156  ·      64960  ·      54058  ·            4  ·          -  │
 ···················|······················|··············|·············|·············|···············|··············
@@ -116,7 +118,7 @@
 ··········································|··············|·············|·············|···············|··············
 |  TestERC20Token                         ·           -  ·          -  ·    1193264  ·          4 %  ·          -  │
 ··········································|··············|·············|·············|···············|··············
-|  VTVLVesting                            ·     3740716  ·    3740728  ·    3740727  ·       12.5 %  ·          -  │
+|  VTVLVesting                            ·     3806813  ·    3806825  ·    3806824  ·       12.7 %  ·          -  │
 ·-----------------------------------------|--------------|-------------|-------------|---------------|-------------·

Use custom errors

Using custom errors can save a significant amount of gas when the function reverts + reduces code size and saves deployment gas.