Lines of code
<https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L418-L437&gt;

Vulnerability details

Impact

When an employee has an active claim, this employee can call the following withdraw function to withdraw the claimable amount that she or he is entitled to, which would increase her or his claim’s amountWithdrawn. Because the employee is free to call withdraw whenever she or he wants, it is possible that her or his claimable amount accumulates while amountWithdrawn remains unchanged in the claim.

<https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L364-L392&gt;

    function withdraw() hasActiveClaim(_msgSender()) external {
        // Get the message sender claim - if any

        Claim storage usrClaim = claims[_msgSender()];

        // 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));

        // Make sure we didn't already withdraw more that we're allowed.
        require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW");

        // Calculate how much can we withdraw (equivalent to the above inequality)
        uint112 amountRemaining = allowance - usrClaim.amountWithdrawn;

        // "Double-entry bookkeeping"
        // Carry out the withdrawal by noting the withdrawn amount, and by transferring the tokens.
        usrClaim.amountWithdrawn += amountRemaining;
        // Reduce the allocated amount since the following transaction pays out so the "debt" gets reduced
        numTokensReservedForVesting -= amountRemaining;
        
        // After the "books" are set, transfer the tokens
        // Reentrancy note - internal vars have been changed by now
        // Also following Checks-effects-interactions pattern
        tokenAddress.safeTransfer(_msgSender(), amountRemaining);

        // Let withdrawal known to everyone.
        emit Claimed(_msgSender(), amountRemaining);
    }

When the employment of the employee just gets terminated, the employee might not be able to call the withdraw function promptly because she or he is busy for other things. Being unaware of that the employee has not withdrawn the claimable amount that she or he has earned, the admin calls the following revokeClaim function for preventing the employee from claiming future amounts that she or he is not entitled to. As a result, even that the admin does not revoke the employee’s claim maliciously, the employee is not able to withdraw the claimable amount that she or he deserves based on her or his time with the organization. This is unfair to the employee for losing the earned claimable amount because she or he has worked for the organization until the moment when her or his employment is terminated.

<https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L418-L437&gt;

    function revokeClaim(address _recipient) external onlyAdmin hasActiveClaim(_recipient) {
        // Fetch the claim
        Claim storage _claim = claims[_recipient];
        // Calculate what the claim should finally vest to
        uint112 finalVestAmt = finalVestedAmount(_recipient);

        // No point in revoking something that has been fully consumed
        // so require that there be unconsumed amount
        require( _claim.amountWithdrawn < finalVestAmt, "NO_UNVESTED_AMOUNT");

        // The amount that is "reclaimed" is equal to the total allocation less what was already withdrawn
        uint112 amountRemaining = finalVestAmt - _claim.amountWithdrawn;

        // Deactivate the claim, and release the appropriate amount of tokens
        _claim.isActive = false;    // This effectively reduces the liability by amountRemaining, so we can reduce the liability numTokensReservedForVesting by that much
        numTokensReservedForVesting -= amountRemaining; // Reduces the allocation

        // Tell everyone a claim has been revoked.
        emit ClaimRevoked(_recipient, amountRemaining, uint40(block.timestamp), _claim);
    }

Proof of Concept

Please append the following test in the Withdraw describe block in test\VTVLVesting.ts. This test will pass to demonstrate the described scenario.

  it("Employee can be unable to withdraw claimable amount that she or he deserves after admin revokes her or his claim", async () => {
    const employee = owner2;

    const {vestingContract} = await createPrefundedVestingContract({tokenName, tokenSymbol, initialSupplyTokens});

    const startTimestamp = await getLastBlockTs();
    const endTimestamp = startTimestamp + 1000;
    const releaseIntervalSecs = 1;
    await vestingContract.createClaim(employee.address, startTimestamp, endTimestamp, 0, releaseIntervalSecs, linearVestAmount, 0);

    await ethers.provider.send("evm_mine", [startTimestamp + 500]);

    // The employee has a positive vested amount at this moment.
    const vestedAmount_ = await vestingContract.connect(employee).vestedAmount(employee.address, startTimestamp + 500);
    expect(vestedAmount_).to.be.gt(0);

    // At this moment, the employee has a positive claimable amount that she or he deserves because she or he has worked for the organization until this moment.
    const claimableAmount_ = await vestingContract.connect(employee).claimableAmount(employee.address);
    expect(claimableAmount_).to.be.gt(0);

    // When the employment of the employee just gets terminated, the employee might not be able to call the withdraw function promptly because she or he is busy for other things.
    // Unknowing that the employee has not withdrawn the amount that she or he deserves, the admin calls the revokeClaim function for preventing the employee from claiming future amounts that she or he is not entitled to.
    // As a result, even that the admin does not revoke the employee's claim maliciously, the employee is not able to withdraw the claimable amount that she or he has earned with her or his time with the organization.
    await (await vestingContract.revokeClaim(employee.address)).wait();
    await expect(vestingContract.connect(employee).withdraw()).to.be.revertedWith("NO_ACTIVE_CLAIM");
  });

Tools Used

VSCode

Recommended Mitigation Steps

Since the admin has the incentive to call the revokeClaim function promptly for preventing the employee from withdrawing the future amounts that she or he is not entitled to, the revokeClaim function can be updated in a way so when calling it, if the claim has a vested amount that is not withdrawn yet, this amount can be stored for the employee to withdraw later; then, numTokensReservedForVesting can be reduced by the claim’s remaining amount, which is resulted after subtracting amountWithdrawn and the vested amount that is not withdrawn from finalVestAmt.

---

The text was updated successfully, but these errors were encountered:

All reactions