VTVL contest - ElKu's results

Lines of code

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

Vulnerability details

Impact

Admin can indirectly withdraw an existing claim by revoking the claim and then using the withdrawAdmin function.

Proof of Concept

As per the VTVL contest page:

No one other than the designated vesting recipient (not even the admin) can withdraw on an existing claim - but the admin can revoke the claim.

The above statement has contradicting elements in it. The admin has the power to revoke a claim by using the revokeClaim function. This is okay, but the revokeClaim function ignores any unclaimed amount of the recipient address.

uint112 amountRemaining = finalVestAmt - _claim.amountWithdrawn;
// and this is subtracted from the state variable as follows:
numTokensReservedForVesting -= amountRemaining;

Whatever is not accounted in the numTokensReservedForVesting variable can be withdrawn by the admin.

As you see in the withdrawAdmin function:

uint256 amountRemaining = tokenAddress.balanceOf(address(this)) - numTokensReservedForVesting;
require(amountRemaining >= _amountRequested, "INSUFFICIENT_BALANCE");

Which means that the unclaimed tokens which should have been belonged to the recipient address, is now added to the free balance of the contract which the admin can transfer back to himself.

This indirectly is a violation of the statement that, not even the admin can withdraw on an existing claim.

I added the following POC, in the VTVLVesting.ts file:

  it("Test for loss of unclaimed tokens on claim revoke", async() => {
    const {tokenContract, vestingContract} = await createPrefundedVestingContract({tokenName, tokenSymbol, initialSupplyTokens});
    const startTimestamp = dateToTs('2023-02-01');
    const month1 = 30 * 24* 60 * 60;
    const releaseIntervalSecs = BigNumber.from(month1); // 1 month 
    const endTimestamp = startTimestamp.add(releaseIntervalSecs.mul(48)); // 4 years 
    const cliffReleaseTimestamp = 0;  // dateToTs(new Date('2023-02-01'));
    const linearVestAmount = ethers.utils.parseUnits('100', 18);
    const cliffAmount = 0; //ethers.utils.parseUnits('10', 18);
    const [recipientAddress] = await ethers.getSigners();

    let tx = await vestingContract.createClaim(recipientAddress.address, startTimestamp, endTimestamp, cliffReleaseTimestamp, releaseIntervalSecs, linearVestAmount, cliffAmount);
    let currentBalance = await tokenContract.balanceOf(recipientAddress.address);
    console.log("initialBalance of claim holder=            ", ethers.utils.formatEther(currentBalance));

    let returnedClaimInfo = await vestingContract.getClaim(recipientAddress.address);
    expect(tx).to.emit(vestingContract, "ClaimCreated").withArgs(recipientAddress.address, returnedClaimInfo);

    await ethers.provider.send("evm_mine", [Number(startTimestamp)+month1*6+10]); //6 months fast forwarded
    let claimableAmount = await vestingContract.claimableAmount(recipientAddress.address);
    console.log("claimableAmount after 6 months=            ", ethers.utils.formatEther(claimableAmount));

    console.log("Admin revokes the account...");
    tx = await vestingContract.revokeClaim(recipientAddress.address);
    claimableAmount = await vestingContract.claimableAmount(recipientAddress.address);
    console.log("claimableAmount after account revoked=     ", ethers.utils.formatEther(claimableAmount));
    currentBalance = await tokenContract.balanceOf(recipientAddress.address);
    console.log("Balance of claim holder after claim revoke=", ethers.utils.formatEther(currentBalance));
  });

The following console.log prints were the result:

initialBalance of claim holder=             0.0
claimableAmount after 6 months=             12.5
Admin revokes the account...
claimableAmount after account revoked=      0.0
Balance of claim holder after claim revoke= 0.0

You can see that after the revoking of the claim, both the claim recipient balance and his claimableAmount are zero.

Tools Used

Manual analysis, Hardhat, VSCode

Recommended Mitigation Steps

In the revokeClaim function, send the unclaimed tokens to the claim owner before deactivating the claim. The code lines from the withdraw() function can be borrowed for this.

This will make sure that the vested address doesn't have to live in fear and withdraw the tokens every time they get released at the end of releaseIntervalsSecs.

Non-critical Issues

1. createClaimsBatch function could be modified to avoid unexpected reverts

Right now, the createClaimsBatch function repeatedly calls the _createClaimUnchecked internal function. Which in turn contains many require statements before a claim is created. If any one of these claim creation's revert, then the whole createClaimsBatch function would revert wasting a significant amount of gas.

I recommend that a try and catch block approach should be used instead. The try block could try creating the claim with the _createClaimUnchecked function and if it reverts, we can add the particular recipient to an address array in the catch block. This list of failed recipient addresses could be returned to the user, which he can check and make necessary corrections so that he can call the batch function again.

2. Missing emit statement in the withdrawOtherToken function

withdrawOtherToken function should have an emit statement to let the user base know that the admin has withdrawn the other tokens which were wrongly sent to the contract.

3. The emit statement should show which particular admin called the function

The following functions doesn't emit information on which admin called the method. I suggest this information to be emitted as well.

1. _createClaimUnchecked
2. revokeClaim