VTVL contest - rbserver's results

Lines of code

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

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

    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

    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.

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/token/VariableSupplyERC20Token.sol#L13-L34 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/token/VariableSupplyERC20Token.sol#L36-L46

Vulnerability details

Impact

When deploying the VariableSupplyERC20Token contract, the token's maximum supply can be specified. As the comments of the following constructor show, the total supply of this token should never exceed the maximum supply if it is specified to be positive.

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/token/VariableSupplyERC20Token.sol#L13-L34

    /**
    @notice A ERC20 token contract that allows minting at will, with limited or unlimited supply. No burning possible
    @dev
    @param name_ - Name of the token
    @param symbol_ - Symbol of the token
    @param initialSupply_ - How much to immediately mint (saves another transaction). If 0, no mint at the beginning.
    @param maxSupply_ - What's the maximum supply. The contract won't allow minting over this amount. Set to 0 for no limit.
    */
    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?
        require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");
        mintableSupply = maxSupply_;
        
        // Note: the check whether initial supply is less than or equal than mintableSupply will happen in mint fn.
        if(initialSupply_ > 0) {
            mint(_msgSender(), initialSupply_);
        }
    }

After deploying the token with a maximum supply that is positive, the following mint function can be called to mint more of this token. It is possible that mintableSupply is decreased to 0 after minting for one or more rounds although mintableSupply was set to maxSupply_ in the constructor. After mintableSupply becomes 0, calling mint can mint an unlimited amount of this token in which the specified maximum supply has no effect at all. Using the deployed VariableSupplyERC20Token contract as the token for the vesting contract, the admin would believe that calling mint would revert if the maximum supply is going to be exceeded and hence can call this function without double checking the current total supply beforehand. When the total supply exceeds the specified maximum supply, the recipients of the vesting token would receive the claimable amounts that are diluted and are worth less than expected. If the recipients knew that the token's total supply is actually unlimited instead of being capped by the specified maximum supply, they might not even enter in this vesting situation in the first place. For example, whether the vesting token's total supply is capped by the specified maximum supply or not can be a deciding factor for the employee to choose to work with the organization or not. Thus, the described scenario is unfair to the recipients of the vesting token.

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/token/VariableSupplyERC20Token.sol#L36-L46

    function mint(address account, uint256 amount) public onlyAdmin {
        require(account != address(0), "INVALID_ADDRESS");
        // If we're using maxSupply, we need to make sure we respect it
        // mintableSupply = 0 means mint at will
        if(mintableSupply > 0) {
            require(amount <= mintableSupply, "INVALID_AMOUNT");
            // We need to reduce the amount only if we're using the limit, if not just leave it be
            mintableSupply -= amount;
        }
        _mint(account, amount);
    }

Proof of Concept

Please add the following code and test in test\token\VariableSupplyERC20Token.ts. This test will pass to demonstrate the described scenario.

import { expect } from "chai";
import { ethers } from "hardhat";

describe("VariableSupplyERC20Token", async function () {
    it("Total supply of VariableSupplyERC20Token can exceed specified maximum supply", async () => {
        const [admin] = await ethers.getSigners();

        // The maximum supply is specified to be 1000, which is more than 0.
        const maxSupply_ = 1000;
        const VariableSupplyERC20TokenFactory = await ethers.getContractFactory('VariableSupplyERC20Token');
        const variableSupplyERC20Token = await VariableSupplyERC20TokenFactory.connect(admin).deploy('Test Token', 'TT', 500, maxSupply_);

        // The total supply is 500 at this moment because the initial supply is specified to be 500, which is less than the specified maximum supply.
        let totalSupply_ = await variableSupplyERC20Token.connect(admin).totalSupply();
        expect(totalSupply_).to.be.eq(500);
        expect(totalSupply_).to.be.lt(maxSupply_);

        // 500 more tokens are minted.
        await variableSupplyERC20Token.connect(admin).mint(admin.address, 500);

        // The total supply reaches the specified maximum supply, which is 1000, at this moment.
        totalSupply_ = await variableSupplyERC20Token.connect(admin).totalSupply();
        expect(totalSupply_).to.be.eq(1000);
        expect(totalSupply_).to.be.eq(maxSupply_);

        // Trying to mint 500 more tokens
        await variableSupplyERC20Token.connect(admin).mint(admin.address, 500);

        // Unexpectedly, the total supply exceeds the specified maximum supply at this moment.
        totalSupply_ = await variableSupplyERC20Token.connect(admin).totalSupply();
        expect(totalSupply_).to.be.eq(1500);
        expect(totalSupply_).to.be.gt(maxSupply_);

        // Trying to mint 500 more tokens again
        await variableSupplyERC20Token.connect(admin).mint(admin.address, 500);

        // The total supply exceeds the specified maximum supply by even more at this moment.
        totalSupply_ = await variableSupplyERC20Token.connect(admin).totalSupply();
        expect(totalSupply_).to.be.eq(2000);
        expect(totalSupply_).to.be.gt(maxSupply_);
    });
});

Tools Used

VSCode

Recommended Mitigation Steps

In the VariableSupplyERC20Token contract, the specified maxSupply_ can be stored in a state variable. When calling the mint function, this state variable can be checked to see if it is more than 0 instead of checking mintableSupply > 0; if so, require(amount <= mintableSupply, "INVALID_AMOUNT"); mintableSupply -= amount; can then be executed.

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L245-L304 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L123-L140 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L418-L437

Vulnerability details

Impact

When the admin is not careful enough, an incorrect claim for the recipient of the vesting token can be created. However, it is not possible to call the following _createClaimUnchecked function again to update the created claim for the recipient because this function uses the hasNoClaim modifier below, which requires _claim.startTimestamp == 0. Calling the revokeClaim function below first and then calling _createClaimUnchecked also cannot update the created claim for the recipient because calling revokeClaim does not reset the claim's startTimestamp. Hence, in this case, the recipient's claimable amounts must depend on the the created claim that is incorrect. If the incorrect claim offers less cliffAmount or linearVestAmount than it should, the recipient will unfairly lose the amount difference that she or he should be entitled to when vesting occurs.

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L245-L304

    function _createClaimUnchecked(
            address _recipient, 
            uint40 _startTimestamp, 
            uint40 _endTimestamp, 
            uint40 _cliffReleaseTimestamp, 
            uint40 _releaseIntervalSecs, 
            uint112 _linearVestAmount, 
            uint112 _cliffAmount
                ) private  hasNoClaim(_recipient) {

        ...

        Claim memory _claim = Claim({
            startTimestamp: _startTimestamp,
            endTimestamp: _endTimestamp,
            cliffReleaseTimestamp: _cliffReleaseTimestamp,
            releaseIntervalSecs: _releaseIntervalSecs,
            cliffAmount: _cliffAmount,
            linearVestAmount: _linearVestAmount,
            amountWithdrawn: 0,
            isActive: true
        });
        // Our total allocation is simply the full sum of the two amounts, _cliffAmount + _linearVestAmount
        // Not necessary to use the more complex logic from _baseVestedAmount
        uint112 allocatedAmount = _cliffAmount + _linearVestAmount;

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

        // Done with checks

        // Effects limited to lines below
        claims[_recipient] = _claim; // store the claim
        numTokensReservedForVesting += allocatedAmount; // track the allocated amount
        vestingRecipients.push(_recipient); // add the vesting recipient to the list
        emit ClaimCreated(_recipient, _claim); // let everyone know
    }

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L123-L140

    modifier hasNoClaim(address _recipient) {
        Claim storage _claim = claims[_recipient];
        // Start timestamp != 0 is a sufficient condition for a claim to exist
        // This is because we only ever add claims (or modify startTs) in the createClaim function 
        // Which requires that its input startTimestamp be nonzero
        // So therefore, a zero value for this indicates the claim does not exist.
        require(_claim.startTimestamp == 0, "CLAIM_ALREADY_EXISTS");
        
        // We don't even need to check for active to be unset, since this function only 
        // determines that a claim hasn't been set
        // require(_claim.isActive == false, "CLAIM_ALREADY_EXISTS");
    
        // Further checks aren't necessary (to save gas), as they're done at creation time (createClaim)
        // require(_claim.endTimestamp == 0, "CLAIM_ALREADY_EXISTS");
        // require(_claim.linearVestAmount + _claim.cliffAmount == 0, "CLAIM_ALREADY_EXISTS");
        // require(_claim.amountWithdrawn == 0, "CLAIM_ALREADY_EXISTS");
        _;
    }

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

    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("Incorrect claim cannot be corrected after it is created", async () => {
    const employee = owner2;

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

    const startTimestamp = await getLastBlockTs();
    const endTimestamp = startTimestamp + 1000;
    const releaseIntervalSecs = 1;

    // The admin is not careful enough and creates an incorrect claim for the employee.
    await vestingContract.createClaim(employee.address, startTimestamp, endTimestamp, 0, releaseIntervalSecs, linearVestAmount, 0);

    // The admin cannot correct the claim by calling createClaim again.
    await expect(
      vestingContract.createClaim(employee.address, startTimestamp, endTimestamp, 0, releaseIntervalSecs, ethers.utils.parseUnits('200', 18), 0)
    ).to.be.revertedWith("CLAIM_ALREADY_EXISTS");

    // After revoking the employee's original claim, the admin still cannot create a new claim that is correct for the employee.
    await (await vestingContract.revokeClaim(employee.address)).wait();
    await expect(
      vestingContract.createClaim(employee.address, startTimestamp, endTimestamp, 0, releaseIntervalSecs, ethers.utils.parseUnits('200', 18), 0)
    ).to.be.revertedWith("CLAIM_ALREADY_EXISTS");
  });

Tools Used

VSCode

Recommended Mitigation Steps

When constructing the VTVLVesting contract, a new state variable can be configured as the buffer time after the claim creation and before the vesting procedure starts for a claim. When _createClaimUnchecked is called, this state variable can be used to make another variable for representing the vesting procedure's start time for the claim, and this variable would be added in the created claim's Claim struct. Moreover, another admin function can be added for updating an existing claim; as long as the current block.timestamp is less than the vesting procedure's start time for the created claim, the admin can call this function to update the claim's Claim struct.

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L245-L304 https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L364-L392

Vulnerability details

Impact

When the following _createClaimUnchecked function is called, the specified cliffAmount and linearVestAmount are recorded for the claim. Later, when calling the withdraw function below that executes tokenAddress.safeTransfer(_msgSender(), amountRemaining), the amountRemaining, which is a part or all of the sum of the recorded cliffAmount and linearVestAmount, will be transferred to the recipient. If the token for the vesting contract is a rebasing token, the amountRemaining can correspond to a higher or lower actual token amount after a rebasing event occurs because of the token's elastic supply in which its circulating supply changes automatically according to the price fluctuations. Yet, the amountRemaining being transferred is not adjusted for the rebasing event, which can be problematic. For example, when creating a claim, the deployed VTVLVesting contract can hold enough token balance for covering the claim's cliffAmount and linearVestAmount but withdrawing an amountRemaining that equals the sum of the claim's cliffAmount and linearVestAmount can revert after a rebasing event that decreases the token's total supply occurs; due to the reversion, the recipient is unable to receive the claimable amount that she or he deserves, which is unfair.

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L245-L304

    function _createClaimUnchecked(
            address _recipient, 
            uint40 _startTimestamp, 
            uint40 _endTimestamp, 
            uint40 _cliffReleaseTimestamp, 
            uint40 _releaseIntervalSecs, 
            uint112 _linearVestAmount, 
            uint112 _cliffAmount
                ) private  hasNoClaim(_recipient) {

        ...

        Claim memory _claim = Claim({
            startTimestamp: _startTimestamp,
            endTimestamp: _endTimestamp,
            cliffReleaseTimestamp: _cliffReleaseTimestamp,
            releaseIntervalSecs: _releaseIntervalSecs,
            cliffAmount: _cliffAmount,
            linearVestAmount: _linearVestAmount,
            amountWithdrawn: 0,
            isActive: true
        });
        // Our total allocation is simply the full sum of the two amounts, _cliffAmount + _linearVestAmount
        // Not necessary to use the more complex logic from _baseVestedAmount
        uint112 allocatedAmount = _cliffAmount + _linearVestAmount;

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

        // Done with checks

        // Effects limited to lines below
        claims[_recipient] = _claim; // store the claim
        numTokensReservedForVesting += allocatedAmount; // track the allocated amount
        vestingRecipients.push(_recipient); // add the vesting recipient to the list
        emit ClaimCreated(_recipient, _claim); // let everyone know
    }

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

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

Proof of Concept

First, please add the following mock rebasing token contract.

//SPDX-License-Identifier: Unlicense
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract MockRebasingToken is ERC20 {
    uint256 private constant INITIAL_SUPPLY_ = 100_000_000 * 10**18;
    uint256 private constant TOTAL_ = type(uint256).max - (type(uint256).max % INITIAL_SUPPLY_);

    uint256 private _totalSupply;
    uint256 private mul_;
    mapping(address => uint256) private balance_;
    constructor(string memory name_, string memory symbol_) ERC20(name_, symbol_) {
        _totalSupply = INITIAL_SUPPLY_;
        balance_[msg.sender] = TOTAL_;
        mul_ = TOTAL_ / _totalSupply;
    }

    function balanceOf(address _addr) public view override returns (uint256) {
        return balance_[_addr] / mul_;
    }

    function transfer(address _to, uint256 _value) public override returns (bool) {
        uint256 value_ = _value * mul_;

        require(balance_[msg.sender] >= value_, "Mock Rebasing Token: transfer amount exceeds balance");
        balance_[msg.sender] = balance_[msg.sender] - value_;
        balance_[_to] = balance_[_to] + value_;

        return true;
    }

    function rebase(int256 _supplyDelta) external returns (uint256) {
        if (_supplyDelta == 0) {
            return _totalSupply;
        }

        if (_supplyDelta < 0) {
            _totalSupply = _totalSupply - uint256(_supplyDelta * (-1));
        } else {
            _totalSupply = _totalSupply + uint256(_supplyDelta);
        }

        mul_ = TOTAL_ / _totalSupply;

        return _totalSupply;
    }
}

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

  it("Using rebasing token as the token for the vesting contract can be problematic", async () => {
    // The mock rebasing token is deployed.
    const MockRebasingTokenFactory = await ethers.getContractFactory('MockRebasingToken');
    const mockRebasingToken = await MockRebasingTokenFactory.deploy("Test Token", "TT");

    // A vesting contract is created by using the mock rebasing token.
    const vestingContract = await deployVestingContract(mockRebasingToken.address);
    await vestingContract.deployed();
    expect(await vestingContract.tokenAddress()).to.be.equal(mockRebasingToken.address);

    // The linearVestAmount of the rebasing token is transferred to the vesting contract.
    await mockRebasingToken.transfer(vestingContract.address, linearVestAmount);

    const employee = owner2;
    const startTimestamp = await getLastBlockTs();
    const endTimestamp = startTimestamp + 1000;
    const releaseIntervalSecs = 1;

    // A claim is created for the employee.
    // Creating this claim does not encounter the INSUFFICIENT_BALANCE reversion because the vesting contract has enough tokens for the claim at this moment.
    await vestingContract.createClaim(employee.address, startTimestamp, endTimestamp, 0, releaseIntervalSecs, linearVestAmount, 0);

    // After a while, a rebasing event occurs that decreases the token's total supply.
    await ethers.provider.send("evm_mine", [startTimestamp + 250]);
    await mockRebasingToken.rebase(-1000);

    // When the employee tries to withdraw her or his vested amount, the withdraw transaction reverts because the vesting contract's token balance becomes insufficient for covering the claim after the rebasing event.
    await ethers.provider.send("evm_mine", [endTimestamp]);
    await expect(
      vestingContract.connect(employee).withdraw()
    ).to.be.revertedWith("Mock Rebasing Token: transfer amount exceeds balance");
  });

Tools Used

VSCode

Recommended Mitigation Steps

For each time that the _createClaimUnchecked function is called, the proportion of the actual rebasing token balance owned by the deployed VTVLVesting contract at that moment that corresponds to the sum of the cliffAmount and linearVestAmount for each created claim can be recorded. When calling the withdraw function, such proportion recorded for the claim can be used to multiply by the actual rebasing token balance owned by the deployed VTVLVesting contract at the new moment to determine the adjusted amount for the sum of the claim's cliffAmount and linearVestAmount; then, the proportion of the sum of the claim's cliffAmount and linearVestAmount that corresponds to the amountRemaining can be multiplied by such adjusted amount to determine the actual transfer amount for executing tokenAddress.safeTransfer.

Alternatively, similar to other protocols, this protocol does not have to support rebasing tokens and can use a blocklist to block the usage of these tokens but does need to clearly communicate about this with the users.

[L-01] FOUNDERS' ADMIN RIGHTS CAN BE REVOKED BY OTHER ADMINS

Usually, the deployer is one of the founders. After the deployment, the deployer can call the following setAdmin function to grant admin rights to other individual, who may not be one of the founders. This requires huge trust between the founders and added admins because these added admins can also call setAdmin to revoke the admin rights from the founders. In case if the organization would want to protect the founders, especially if the founders are the ones who provide fundings or attract fundings, please consider creating a founder role and make setAdmin callable only by the addresses with this founder role.

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

    function setAdmin(address admin, bool isEnabled) public onlyAdmin {
        require(admin != address(0), "INVALID_ADDRESS");
        _admins[admin] = isEnabled;
        emit AdminAccessSet(admin, isEnabled);
    }

[L-02] linearVestAmount IS NOT REQUIRED TO BE SET BUT startTimestamp IS

When calling the following _createClaimUnchecked function, it is possible to create a claim with 0 linearVestAmount, which is also confirmed by the documentation that states: "Each of the parts (cliff and linear) have amounts that can be allocated to each. The founders can opt to use either or both options for each of the claims." However, startTimestamp is required to be set for the claim due to require(_startTimestamp > 0, "INVALID_START_TIMESTAMP"), which is somewhat inconsistent. If this is intended for the design, then this requirement needs to be clearly communicated with the users of the protocol. Otherwise, the protocol can allow claims to be created without setting startTimestamp and update the implementations, such as the following hasActiveClaim modifier, to not depend on claim's startTimestamp in some occasions.

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L245-L304

    function _createClaimUnchecked(
            address _recipient, 
            uint40 _startTimestamp, 
            uint40 _endTimestamp, 
            uint40 _cliffReleaseTimestamp, 
            uint40 _releaseIntervalSecs, 
            uint112 _linearVestAmount, 
            uint112 _cliffAmount
                ) private  hasNoClaim(_recipient) {

        require(_recipient != address(0), "INVALID_ADDRESS");
        require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT"); // Actually only one of linearvested/cliff amount must be 0, not necessarily both
        require(_startTimestamp > 0, "INVALID_START_TIMESTAMP");

        ...

	}

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L105-L117

    modifier hasActiveClaim(address _recipient) {
        Claim storage _claim = claims[_recipient];
        require(_claim.startTimestamp > 0, "NO_ACTIVE_CLAIM");

        // We however still need the active check, since (due to the name of the function)
        // we want to only allow active claims
        require(_claim.isActive == true, "NO_ACTIVE_CLAIM");

        // Save gas, omit further checks
        // require(_claim.linearVestAmount + _claim.cliffAmount > 0, "INVALID_VESTED_AMOUNT");
        // require(_claim.endTimestamp > 0, "NO_END_TIMESTAMP");
        _;
    }

[L-03] UNRESOLVED TODO AND QUESTION COMMENTS

Comments regarding todos and unanswered questions indicate that there are unresolved action items for implementation, which need to be addressed before the protocol deployment. Please review the todo and question comments in the following code.

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

        // Potential TODO: sanity check, if _linearVestAmount == 0, should we perhaps force that start and end ts are the same?

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/token/VariableSupplyERC20Token.sol#L22-L26

        // 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?

[N-01] tokenAddress CAN BE RENAMED

In the following constructor, _tokenAddress and tokenAddress are not exactly the token's address. To be more accurate and to avoid confusion, tokenAddress can be renamed to something like token.

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L81-L84

    constructor(IERC20 _tokenAddress) {
        require(address(_tokenAddress) != address(0), "INVALID_ADDRESS");
        tokenAddress = _tokenAddress;
    }

[N-02] INCORRECT AND REDUNDANT COMMENT

The second comment for uint112 range in the following code is redundant and also incorrect. To avoid confusion, it can be removed.

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L40-L41

        // uint112 range: range 0 –     5,192,296,858,534,827,628,530,496,329,220,095.
        // uint112 range: range 0 –                             5,192,296,858,534,827.

[N-03] COMMENTED OUT CODE CAN BE REMOVED

The following code is commented out. To improve readability and maintainability, please consider removing it.

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/token/FullPremintERC20Token.sol#L9

    // uint constant _initialSupply = 100 * (10**18);

[N-04] MISSING NATSPEC COMMENTS

NatSpec comments provide rich code documentation. NatSpec comments are missing for the following functions. Please consider adding them.

contracts\AccessProtected.sol
  29: function isAdmin(address _addressToCheck) external view returns (bool) {

contracts\token\VariableSupplyERC20Token.sol
  36: function mint(address account, uint256 amount) public onlyAdmin {

[N-05] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. @param or @return comments are missing for the following functions. Please consider completing NatSpec comments for them.

contracts\VTVLVesting.sol
  91: function getClaim(address _recipient) external view returns (Claim memory) {    
  147: function _baseVestedAmount(Claim memory _claim, uint40 _referenceTs) internal pure returns (uint112) {  
  196: function vestedAmount(address _recipient, uint40 _referenceTs) public view returns (uint112) {  
  206: function finalVestedAmount(address _recipient) public view returns (uint112) {  
  215: function claimableAmount(address _recipient) external view returns (uint112) {  
  223: function allVestingRecipients() external view returns (address[] memory) {  
  230: function numVestingRecipients() external view returns (uint256) {  
  333: function createClaimsBatch( 

[N-06] FLOATING PRAGMAS

It is a best practice to lock pragmas instead of using floating pragmas to ensure that contracts are tested and deployed with the intended compiler version. Accidentally deploying contracts with different compiler versions can lead to unexpected risks and undiscovered bugs. Please consider locking pragma for the VariableSupplyERC20Token contract.

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/token/VariableSupplyERC20Token.sol#L2

pragma solidity ^0.8.14;

[G-01] SOME OPERATIONS CAN BE SKIPPED WHEN CLAIM'S linearVestAmount IS 0

It is possible to create a claim with 0 linearVestAmount; this is confirmed by the documentation as well, which states: "Each of the parts (cliff and linear) have amounts that can be allocated to each. The founders can opt to use either or both options for each of the claims." When linearVestAmount is 0, vestAmt would be increased by 0 after executing the operations within the _referenceTs > _claim.startTimestamp if block in the following _baseVestedAmount function. Hence, these operations can be conditionally skipped if linearVestAmount is 0 to save gas.

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L147-L188

    function _baseVestedAmount(Claim memory _claim, uint40 _referenceTs) internal pure returns (uint112) {
        uint112 vestAmt = 0;
        
        // the condition to have anything vested is to be active
        if(_claim.isActive) {
            // no point of looking past the endTimestamp as nothing should vest afterwards
            // So if we're past the end, just get the ref frame back to the end
            if(_referenceTs > _claim.endTimestamp) {
                _referenceTs = _claim.endTimestamp;
            }

            // If we're past the cliffReleaseTimestamp, we release the cliffAmount
            // We don't check here that cliffReleaseTimestamp is after the startTimestamp 
            if(_referenceTs >= _claim.cliffReleaseTimestamp) {
                vestAmt += _claim.cliffAmount;
            }

            // 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) {
                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;
            }
        }
        
        // Return the bigger of (vestAmt, _claim.amountWithdrawn)
        // Rationale: no matter how we calculate the vestAmt, we can never return that less was vested than was already withdrawn.
        // Case where this could be relevant - If the claim is inactive, vestAmt would be 0, yet if something was already withdrawn 
        // on that claim, we want to return that as vested thus far - as we want the function to be monotonic.
        return (vestAmt > _claim.amountWithdrawn) ? vestAmt : _claim.amountWithdrawn;
    }

[G-02] usrClaim.amountWithdrawn CAN BE CACHED IN MEMORY

usrClaim.amountWithdrawn in the following code can be cached in memory, and the cached value can then be accessed. This can save 2 sload operations by using 1 sload, 1 mstore, and 2 mload operations.

https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L373-L377

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

[G-03] BOOLEAN VARIABLE DOES NOT NEED TO BE COMPARED TO BOOLEAN VALUE

Comparing a boolean variable to a boolean value costs more gas than directly checking the boolean variable value.

require(_claim.isActive, "NO_ACTIVE_CLAIM") can be implemented instead of the following code. https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L111

        require(_claim.isActive == true, "NO_ACTIVE_CLAIM");

[G-04] ARITHMETIC OPERATIONS THAT DO NOT UNDERFLOW CAN BE UNCHECKED

Explicitly unchecking arithmetic operations that do not underflow by wrapping these in unchecked {} costs less gas than implicitly checking these.

_referenceTs - _claim.startTimestamp can be unchecked in the following code because _referenceTs > _claim.startTimestamp must be true beforehand. https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L166-L167

            if(_referenceTs > _claim.startTimestamp) {
                uint40 currentVestingDurationSecs = _referenceTs - _claim.startTimestamp; // How long since the start

allowance - usrClaim.amountWithdrawn can be unchecked in the following code because allowance > usrClaim.amountWithdrawn must be true beforehand. https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L374-L377

        require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW");

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

finalVestAmt - _claim.amountWithdrawn can be unchecked in the following code because _claim.amountWithdrawn < finalVestAmt must be true beforehand. https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L426-L429

        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;

[G-05] ARITHMETIC OPERATIONS THAT DO NOT OVERFLOW CAN BE UNCHECKED

Explicitly unchecking arithmetic operations that do not overflow by wrapping these in unchecked {} costs less gas than implicitly checking these.

For the following loop, unchecked {++i} at the end of the loop block can be used, where i is the counter variable. https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L353-L355

        for (uint256 i = 0; i < length; i++) {
            _createClaimUnchecked(_recipients[i], _startTimestamps[i], _endTimestamps[i], _cliffReleaseTimestamps[i], _releaseIntervalsSecs[i], _linearVestAmounts[i], _cliffAmounts[i]);
        }

[G-06] VARIABLE DOES NOT NEED TO BE INITIALIZED TO ITS DEFAULT VALUE

Explicitly initializing a variable with its default value costs more gas than uninitializing it.

numTokensReservedForVesting does not need to be initialized to its default value in the following code. https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L27

    uint112 public numTokensReservedForVesting = 0;

uint256 i can be used instead of uint256 i = 0 in the following code. https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L353-L355

        for (uint256 i = 0; i < length; i++) {
            _createClaimUnchecked(_recipients[i], _startTimestamps[i], _endTimestamps[i], _cliffReleaseTimestamps[i], _releaseIntervalsSecs[i], _linearVestAmounts[i], _cliffAmounts[i]);
        }

[G-07] ++VARIABLE CAN BE USED INSTEAD OF VARIABLE++

++variable costs less gas than variable++. For example, i++ can be changed to ++i in the following code. https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L353-L355

        for (uint256 i = 0; i < length; i++) {
            _createClaimUnchecked(_recipients[i], _startTimestamps[i], _endTimestamps[i], _cliffReleaseTimestamps[i], _releaseIntervalsSecs[i], _linearVestAmounts[i], _cliffAmounts[i]);
        }

[G-08] X = X + Y CAN BE USED INSTEAD OF X += Y

x = x + y costs less gas than x += y. For example, vestAmt += linearVestAmount can be changed to vestAmt = vestAmt + linearVestAmount in the following code.

contracts\VTVLVesting.sol
  161: vestAmt += _claim.cliffAmount;
  179: vestAmt += linearVestAmount;
  179: vestAmt += linearVestAmount;
  301: numTokensReservedForVesting += allocatedAmount; // track the allocated amount
  380: usrClaim.amountWithdrawn += amountRemaining;

[G-09] X = X - Y CAN BE USED INSTEAD OF X -= Y

x = x - y costs less gas than x -= y. For example, mintableSupply -= amount can be changed to mintableSupply = mintableSupply - amount in the following code.

contracts\VTVLVesting.sol
  382: numTokensReservedForVesting -= amountRemaining;
  382: numTokensReservedForVesting -= amountRemaining;
  432: numTokensReservedForVesting -= amountRemaining; // Reduces the allocation

contracts\token\VariableSupplyERC20Token.sol
  43: mintableSupply -= amount;

[G-10] REVERT WITH CUSTOM ERROR CAN BE USED INSTEAD OF REQUIRE() WITH REASON STRING

revert with custom error can cost less gas than require() with reason string. Please consider using revert with custom error to replace the following require().

contracts\AccessProtected.sol
  25: require(_admins[_msgSender()], "ADMIN_ACCESS_REQUIRED");
  40: require(admin != address(0), "INVALID_ADDRESS");

contracts\VTVLVesting.sol
  82: require(address(_tokenAddress) != address(0), "INVALID_ADDRESS");
  107: require(_claim.startTimestamp > 0, "NO_ACTIVE_CLAIM");
  111: require(_claim.isActive == true, "NO_ACTIVE_CLAIM");
  129: require(_claim.startTimestamp == 0, "CLAIM_ALREADY_EXISTS");
  255: require(_recipient != address(0), "INVALID_ADDRESS");
  256: require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT"); // Actually only one of linearvested/cliff amount must be 0, not necessarily both
  257: require(_startTimestamp > 0, "INVALID_START_TIMESTAMP");
  262: require(_startTimestamp < _endTimestamp, "INVALID_END_TIMESTAMP"); // _endTimestamp must be after _startTimestamp
  263: require(_releaseIntervalSecs > 0, "INVALID_RELEASE_INTERVAL");
  264: require((_endTimestamp - _startTimestamp) % _releaseIntervalSecs == 0, "INVALID_INTERVAL_LENGTH");
  270: require( 
  295: require(tokenAddress.balanceOf(address(this)) >= numTokensReservedForVesting + allocatedAmount, "INSUFFICIENT_BALANCE");
  344: require(_startTimestamps.length == length &&
  373: require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW");
  401: require(amountRemaining >= _amountRequested, "INSUFFICIENT_BALANCE");
  425: require( _claim.amountWithdrawn < finalVestAmt, "NO_UNVESTED_AMOUNT");
  446: require(_otherTokenAddress != tokenAddress, "INVALID_TOKEN"); // tokenAddress address is already sure to be nonzero due to constructor
  448: require(bal > 0, "INSUFFICIENT_BALANCE");

contracts\token\FullPremintERC20Token.sol
  11: require(supply_ > 0, "NO_ZERO_MINT");

contracts\token\VariableSupplyERC20Token.sol
  27: require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");
  37: require(account != address(0), "INVALID_ADDRESS");
  41: require(amount <= mintableSupply, "INVALID_AMOUNT");