VTVL contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

The admin can revoke a claim. When doing that, they revoke everything that was not withdrawn by the user. Instead, it should revoke everything that has not been vested yet.

Let's say you're an employee with the standard vestment plan, i.e. 4-year vestment with a 1-year cliff. You leave the company after 2 years. Within that time you've never claimed anything. The admin (your boss), revokes your claim after you've left. Now you don't get anything although you were entitled to 50% of the tokens.

I don't think that users will claim their funds regularly. Instead, they will probably withdraw a large amount at the end. The fact that you lose access to all of your funds when the admin revokes your claim is not clear. Likely, someone won't claim everything on the last day of their job. They will probably do it at some point in the future, believing that they always have access to the funds that are already vested.

It's, in my opinion, a likely scenario that will cause a loss of funds for users. The admin is expected to call revokeClaim() after the employee leaves. The employee is not expected to withdraw their tokens before they leave. That increases the likelihood of it happening.

Proof of Concept

Admin revokes the funds are set's the claim's status to inactive: https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L422-L433

It removes all the funds that were not withdrawn from the internal bookkeeping of funds that are locked up: https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L433

The admin is now able to withdraw those funds using the withdrawAdmin() function: https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L398

The user won't be able to access their funds because the modifier on the withdraw() function: https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L364

Also, if the modifier is removed, the _baseVestedAmount() value won't return their actual vested amount but the amount that was withdrawn because of the following check: https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L151

Tools Used

none

Recommended Mitigation Steps

To fix this, you need a couple of larger changes. Instead of using the isActive property, you should replace it with something like revokedTimestamp. If the value is 0, the claim is active. If the value is not, the claim was revoked. The _baseVestedAmount() function should then return the vested amount until the revokedTimestamp. The funds that are vested before the revokement should still be accessible by the user. Meaning, the withdraw() function should allow them to get those tokens even after the claim was revoked.

Lines of code

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

Vulnerability details

Impact

The contract allows you to specify the mintableSupply. It's supposed to be the total supply of tokens that can be minted and distributed. But, the mint() function ignores the value and allows you to mint more than that. That breaks one of the main invariants of the contract.

It allows the admin to mint more tokens than originally accounted for. That will dilute the supply of previous token recipients resulting in a loss of value for them.

Proof of Concept

    function mint(address account, uint256 amount) public onlyAdmin {
        require(account != address(0), "INVALID_ADDRESS");
        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);
    }

The admin can mint exactly mintableSupply tokens so that the value becomes 0. After that, the if clause won't be true anymore. Thus, subsequent mints will only execute _mint(account, amount). That allows you to mint an unlimited number of tokens.

Step by step:

1. admin creates contract with mintableSupply == 1000
2. admin mints 1000 tokens through the mint() function
3. admin can now mint unlimited tokens because if clause doesn't evaluate to true anymore

Tools Used

none

Recommended Mitigation Steps

Put the _mint() line inside the if block:

    function mint(address account, uint256 amount) public onlyAdmin {
        require(account != address(0), "INVALID_ADDRESS");
        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);
        }
    }

Lines of code

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

Vulnerability details

Impact

It's impossible to create another claim for the same user after the previous one was revoked. A likely scenario might be that the admin creates a claim, sees that there was an issue with it, and revokes it. Now they are not able to create another one for the recipient.

It's also impossible to create a new claim for a user after their previous one is fully vested. A likely scenario is an employee negotiating a new compensation package after the initial one is finished. The admin is not able to provide a new claim for the same address.

While it's still possible for the user to create a new address and receive the tokens there, it's a big downgrade in usability.

The contract is supposed to be part of a "token management platform for web3 founders". In its current state, it lacks basic features to be usable in production.

Each address should be able to have multiple claims at the same time. Those can be active or inactive. It shouldn't matter. From a technical standpoint, it doesn't create any substantial amount of complexity. You simply have to identify each claim through a separate ID instead of the user's address. Limiting each address to only a single claim seems to be arbitrary. Especially considering that there's no reasoning in the documentation on why that choice was made.

As this is severely limiting the business logic of the contract & project, being a "token management platform for web3 founders", I decided to submit it as a MED. It's not a DOS in the traditional sense. The issues just make the contract very limited in its use.

Proof of Concept

When creating a new claim, the hasNoClaim() modifier is triggered: https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L253

The modifier verifies that there's no claim by checking whether the startTimestamp property is 0: https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L129

When an admin revokes a claim, the user's claim is not deleted. Instead, the isActive property is set to false. The startTimestamp is left untouched.

Thus, subsequent tx where a new claim is created for the same address will fail. There's a hard limit of a single claim per address.

Tools Used

none

Recommended Mitigation Steps

Instead of identifying claims through the user's address, use the custom ID. For example, a simple counter that you increment any time a new claim is created. Inside the claim, you should store the owner's address. In your existing modifiers, you verify that msg.sender == claimOwner. Those are the only major changes. The rest of it is just small stuff like checking whether the vestingRecipient already exists or not.

That should allow you to have multiple claims for each address.

Report

• Low
  • L-01: hot wallet has admin rights
  • L-02: documentation about security of funds is misleading

Low

L-01: hot wallet has admin rights

It's bad practice to grant a hot wallet admin rights. These are generally less secure than a multisig or timelock contracts. There's a real risk of these keys being compromised. They generally tend to sit on a machine of one of the devs because they are used for deployment.

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

You should make it very clear that the admin rights should NOT be given to the hot wallet or at least removed immediately after the deployment of the contracts. Instead, they should use a multisig.

L-02: documentation about security of funds is misleading

Inside the contest's readme, you state that:

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

https://github.com/code-423n4/2022-09-vtvl#vesting-recipients

This makes it sound like that the admin can only revoke your access to the tokens. Effectively, those tokens would then be locked up in the contract. But, they can withdraw them after revoking the user's access. So the admin is able to withdraw the tokens from an existing claim.

I understand the reason behind the functionality. I just believe that that part should be reworded in the official documentation.

Gas Report

• G-01: remove unnecessary require statements that can be caught by overflows
• G-02: only compute vested amount if the claim's linearVestAmount is not 0
• G-03: finalVestedAmount() doesn't need to compute vested amount
• G-04: hasActiveClaim() doesn't need to check claim's startTimestamp
• G-05: hasActiveClaim() modifier unnecessary for withdraw()
• G-06: working with uints smaller than 256 is more expensive

G-01: remove unnecessary require statements that can be caught by overflows

If you're going to do A - B, there's no need to check whether A > B in a prior statement. The A - B will simply underflow and revert. It only makes sense if you want to include a detailed error message. In that case, it makes sense to put the A - B statement into an unchecked block to prevent the underlying underflow check. Both options will save gas. It's simply a matter of taste:

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

G-02: only compute vested amount if the claim's linearVestAmount is not 0

Since linearVestAmount can be 0, the whole computation of the vested amount will also be 0 in that case. The computation is done with every call unless it's called before the vesting period starts. User's who don't have any vested amount but just the cliffAmount can save a lot of gas if you don't execute the vesting logic:

function _baseVestedAmount(Claim memory _claim, uint40 _referenceTs) internal pure returns (uint112) {
    // ... rest of logic
        if(_referenceTs > _claim.startTimestamp && _claim.linearVestAmount != 0) {
            // ... rest of logic
        }
    }
}

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

G-03: finalVestedAmount() doesn't need to compute vested amount

You already know what the final vested amount will be: _claim.linearVestAmount + _claim.cliffAmount. Just return that instead of computing it using _baseVestedAmount()

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

G-04: hasActiveClaim() doesn't need to check claim's startTimestamp

You already check whether isActive is true. That's enough to know whether there's an active claim or not. Your reason to check for startTimestamp is:

[...] we only ever set startTimestamp in createClaim, and we never change it. Therefore, startTimestamp will be set IFF a claim has been created.

That's the same for isActive. The property is only set to true when creating a claim.

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

G-05: hasActiveClaim() modifier unnecessary for withdraw()

Inside the withdraw() function, you call vestedAmount(). Inside there, you already check whether the user's claim is active. If it's not, the function returns 0 or amountWithdrawn. With either value, the withdraw() function will revert because of the require statement or the resulting underflow here.

function withdraw() hasActiveClaim(_msgSender()) external {
    // three scenarios:
    // - no claim at all (not initialized)
    // - inactive claim
    // - active claim (can ignore this scenario because that's the modifier allows anyways)
    Claim storage usrClaim = claims[_msgSender()];
    
    // if there's no claim, this will return `0` because all the underlying values are `0`.
    // If the claim is inactive, it will return either `0` or `claim.amountWithdrawn`, see `_baseVestedAmount()` function
    uint112 allowance = vestedAmount(_msgSender(), uint40(block.timestamp));

    // Either way, the maximum value that `allowance` has is `usrClaim.amountWithdrawn` when there's no claim or it's inactive.
    // This check will fail and the function will revert.
    require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW");

    // ... rest of logic
}

Removing the modifier will save you the SLOAD where you check the claim's timestamp/isActive status.

G-06: working with uints smaller than 256 is more expensive

While packing structs can save gas, it's more expensive to work with uints smaller than 256. The EVM works with 32 byte values. If the value is smaller than that, it has to adjust the size of the element to operate on it. That costs gas.

For example, in the _baseVestedAmount() function you work with uints that are all smaller than 256 bits. Doing the same math with uint256 and converting to the smaller type at the end should save you gas: https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L147-L187