VTVL contest - obront's results

Lines of code

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

Vulnerability details

Impact

When a claim is revoked, all pending withdrawals the user has already earned are pulled back.

Since the docs state that the purpose of revoking claims is to allow for early employment termination, the intended behavior should be to allow the employee to withdraw what they have already earned, and revoke all earnings going forward.

There shouldn't be a discrepancy in earnings based on how often a user withdraws their funds, and users should be comfortable leaving their earned funds in the platform without putting them at risk.

Proof of Concept

In revokeClaim(), the amountRemaining is calculated by subtracting the amount withdrawn from the total vesting amount. The claim is then set to inactive, which locks withdrawals going forward.

However, the amount withdrawn does not represent the full amount they have earned, as it does not included the vestedAmount() for the period since their last withdrawal.

For any users who have earned income in their account (ie if they had called withdraw() before the claim was revoked, they had income due to withdraw), revoking their claim will effectively steal that income.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Implement logic similar to withdraw() at the beginning of revokeClaim(): checking the user's vested amount, paying it out, and adjusting their balances.

Lines of code

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

Vulnerability details

Impact

When maxSupply is set in the VariableSupplyERC20Token, the expectation is that this is the maximum number of tokens that can be minted.

However, since this supply cap is only checked when mintableSupply > 0, the result is that once the mintableSupply has been reduced to zero, the supply cap will turn off and unlimited tokens will be available to mint.

Proof of Concept

In the mint() function, the following logic checks the supply cap before minting

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

As you can see, the logic is only checked if mintableSupply > 0. This is intended to have 0 used as an "override" value that allows for unlimited minting.

However, mintableSupply is adjusted as new tokens are minted, and represents the number of tokens remaining in the supply cap. Once maxSupply tokens have been minted, the mintableSupply will then be set to 0, and the check will be skipped, allowing unlimited tokens to be minted.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Instead of using 0 as the variable that bypasses the check, use type(uint256).max.

Lines of code

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

Vulnerability details

Impact

There are many situations where an admin may want to start a new claim for a user with a past claim. For example, they may want to offer an employee an additional pool of options to vest, or they may have revoked a claim accidentally, or they may be rehiring an employee who was previously terminated.

In these cases, the protocol does not allow for them to start a new claim for this user.

Proof of Concept

When _createClaimUnchecked() is called to create a new claim for a user, it includes the modifier hasNoClaim(_recipient).

This check requires that require(_claim.startTimestamp == 0, "CLAIM_ALREADY_EXISTS");

The claim.startTimestamp is never edited in the protocol. It is set when the claim is created and never changes. Furthermore, claims are stored by address, with only one claim permitted per user.

As a result, admins will not be able to create a new claim for any user who has previously had one.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

The more complex but thorough solution is to create an architecture that allows for users to have multiple simultaneous claims.

The simpler solution would be to adjust revokeClaim() so that it resets all information about the claim so that the user has a fresh account for future opportunities. Similarly, withdraw() could contain logic so that, upon their final withdrawal, a user's account is cleared.

Lines of code

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

Vulnerability details

Impact

Admin privileges are stored as a mapping of multiple admins to a boolean flag. Any admin is able to call setAdmin(), changing the boolean flag for themselves or for any other admin.

This architecture creates the possibility that the final admin could accidentally revoke their own admin capabilities, resulting in no admin able to grant privileges and restore the protocol.

Proof of Concept

setAdmin() uses the following logic to set whether a user is an admin or not:

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

This function checks if the admin being set is address(0), but doesn't perform any other check to ensure that the function is safe.

If the final admin were to call setAdmin(myAddress, false), the result would be that the protocol would no longer be functional.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

There are many ways to address this, but the simplest is to not allow an admin to change their own privileges. This ensures that there is always another admin who retains privileges.

Since only admins are able to call this function, the only change that could be made would be to revoke the role, so you can perform a simple check like:

require(admin != _msgSender() "CAN'T REVOKE OWN ADMIN ROLE");