VTVL contest - pcarranzav's results

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/26dda235d38d0f870c1741c9f7eef03229172bbe/contracts/VTVLVesting.sol#L419-L436

Vulnerability details

Impact

In most vesting contracts, once a certain amount has been vested, it belongs to the vesting recipient; and this is independent to whether the vested amount has been withdrawn or not. I would expect that if I have a claim, and it's revoked, I can still withdraw the funds that I had vested until the time of the revocation.

In the VTVL implementation, however, a revocation revokes all the tokens that haven't been withdrawn, independent to whether they have vested or not.

I understand that you want to give admins ability to revoke a claim, but I believe this should only revoke the unvested amount, rather than the amount that hasn't been withdrawn. Otherwise an employee is forced to constantly withdraw the vested amount (spending a lot of gas!) or they risk getting rugpulled by the admins.

Moreover, this allows admins to frontrun the recipients whenever they're trying to withdraw, even after the endTimestamp.

If this is the intended design, I would suggest making documentation super clear about this, as I believe most recipients of a vesting contract would expect their already-vested funds to be safe after a revocation. But even if that is the case, I think the fact that this ability to revoke persisting after endTimestamp is sufficient to consider this a Medium risk issue.

Just to clarify, I understand the contest description specifies:

Therefore any potential risks or attack vectors concerning unauthorised access to admin account(s) (e.g. lost of admin's private key) <u>will not</u> be considered as valid risk findings.

However, I'm not describing a scenario where the admin account is compromised - I'm simply stating that the current implementation does not produce the trustless vesting as described, but rather requires an inordinate (and unnecessary) amount of trust on the admin, and gives admins a way to completely override the vesting contract if an unassuming employee doesn't regularly withdraw.

Proof of Concept

The implementation of this test is actually a proof of concept in itself. I would expect instead that the user should be able to withdraw the vested amount, that corresponds to the interval between startTimestamp and startTimestamp + 500.

Tools Used

VSCode

Recommended Mitigation Steps

Fixing this is not straightforward as it requires changing how revocation works - since revocation marks the claim as inactive, and withdrawals aren't allowed when the claim is inactive, there's no easy way for a user to claim the vested tokens after revocation.

There are three ways around this that I can think of:

a) When revoking a claim, store a revocationTimestamp in the claim struct - the vested amount calculation would then use that as a maximum value for the referenceTs, allowing the user to withdraw the vested amount up to that time, b) When revoking a claim, adjust the endTimestamp, linearVestAmount and cliffAmount so that the total vested amount matches the vested value up to the current block timestamp.

In both cases (a and b) the withdraw function would have to be modified to allow withdrawing with an inactive claim if it still has vested-but-not-withdrawn tokens. The calculation for the vested amount should also return the correct value for an inactive claim that has vested tokens.

The following diff is an example implementation of option b. It's not quite optimal probably, and tests would have to be adjusted accordingly. There might be a few other things to adjust but I think this should do the trick:

diff --git a/contracts/VTVLVesting.sol b/contracts/VTVLVesting.sol
index c564c6f..2a834bb 100644
--- a/contracts/VTVLVesting.sol
+++ b/contracts/VTVLVesting.sol
@@ -147,44 +147,41 @@ contract VTVLVesting is Context, AccessProtected {
     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;
-            }
+        if (_claim.startTimestamp == 0) {
+            return 0;
+        }
+        // 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;
+        // Return vestAmt - it will always be bigger than the withdrawn amount
+        return vestAmt;
     }
 
     /**
@@ -359,12 +356,13 @@ contract VTVLVesting is Context, AccessProtected {
 
     /**
     @notice Withdraw the full claimable balance.
-    @dev hasActiveClaim throws off anyone without a claim.
+    @dev Throws off anyone without a claim.
      */
-    function withdraw() hasActiveClaim(_msgSender()) external {
+    function withdraw() external {
         // Get the message sender claim - if any
 
         Claim storage usrClaim = claims[_msgSender()];
+        require(usrClaim.startTimestamp > 0; "NO_CLAIM");
 
         // 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.
@@ -419,14 +417,27 @@ contract VTVLVesting is Context, AccessProtected {
         // Fetch the claim
         Claim storage _claim = claims[_recipient];
         // Calculate what the claim should finally vest to
+        uint40 currentTs = uint40(block.timestamp);
         uint112 finalVestAmt = finalVestedAmount(_recipient);
+        uint112 alreadyVestedAmt = vestedAmount(_recipient, currentTs);
 
         // No point in revoking something that has been fully consumed
         // so require that there be unconsumed amount
-        require( _claim.amountWithdrawn < finalVestAmt, "NO_UNVESTED_AMOUNT");
+        require(alreadyVestedAmt < 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;
+        uint112 amountRemaining = finalVestAmt - alreadyVestedAmt;
+
+        // Now update the claim so that the total vested amount matches the amount up to this block
+        if (currentTs < _claim.cliffReleaseTimestamp) {
+            _claim.cliffAmount = 0;
+        }
+        if (currentTs <= _claim.startTimestamp) {
+            _claim.linearVestAmount = 0;
+        } else {
+            _claim.linearVestAmount = alreadyVestedAmt - _claim.cliffAmount;
+            _claim.endTimestamp = currentTs;
+        }
 
         // 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

c) A third option would be for the revocation to transfer the outstanding amount to the recipient. This would be a bit simpler, with the only consequence that the admin will now pay the gas for the "withdrawal". Again, this is an example implementation which could be improved / optimized, but illustrates the mitigation:

diff --git a/contracts/VTVLVesting.sol b/contracts/VTVLVesting.sol
index c564c6f..a5aa5ea 100644
--- a/contracts/VTVLVesting.sol
+++ b/contracts/VTVLVesting.sol
@@ -376,19 +376,22 @@ contract VTVLVesting is Context, AccessProtected {
         // 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;
+        _withdraw(usrClaim, _msgSender(), amountRemaining);
+    }
+
+    function _withdraw(Claim storage usrClaim, address recipient, uint112 amount) internal {
         // Reduce the allocated amount since the following transaction pays out so the "debt" gets reduced
-        numTokensReservedForVesting -= amountRemaining;
+        numTokensReservedForVesting -= amount;
+        // Carry out the withdrawal by noting the withdrawn amount, and by transferring the tokens.
+        usrClaim.amountWithdrawn += amount;
         
         // 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);
+        tokenAddress.safeTransfer(recipient, amount);
 
         // Let withdrawal known to everyone.
-        emit Claimed(_msgSender(), amountRemaining);
+        emit Claimed(recipient, amount);
     }
 
     /**
@@ -418,20 +421,26 @@ contract VTVLVesting is Context, AccessProtected {
     function revokeClaim(address _recipient) external onlyAdmin hasActiveClaim(_recipient) {
         // Fetch the claim
         Claim storage _claim = claims[_recipient];
+        uint40 currentTs = uint40(block.timestamp);
         // Calculate what the claim should finally vest to
         uint112 finalVestAmt = finalVestedAmount(_recipient);
+        uint112 actualVestedAmount = vestedAmount(_recipient, currentTs);
 
         // No point in revoking something that has been fully consumed
         // so require that there be unconsumed amount
-        require( _claim.amountWithdrawn < finalVestAmt, "NO_UNVESTED_AMOUNT");
+        require( actualVestedAmount < 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;
+        // The amount that is "reclaimed" is equal to the total allocation less what was already vested
+        uint112 amountRemaining = finalVestAmt - actualVestedAmount;
 
         // 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
 
+        uint112 outstandingAmount = actualVestedAmount - _claim.amountWithdrawn;
+        if (outstandingAmount > 0) {
+            _withdraw(_claim, _recipient, outstandingAmount);
+        }
         // Tell everyone a claim has been revoked.
         emit ClaimRevoked(_recipient, amountRemaining, uint40(block.timestamp), _claim);
     }

I think option c is the cleanest of the three, and requires the least amount of changes in the rest of the contract, so this is the one that I would recommend.

Lines of code

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

Vulnerability details

Impact

Users of the VariableSupplyERC20Token contract would expect the maxSupply_ specified in the constructor to be the maximum amount of tokens that can ever be minted. However, there is a way for the admin to work around this and mint an unlimited amount of tokens, even when maxSupply_ was set to a nonzero value.

Proof of Concept

The issue is evident in the following code:

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

The check for amount <= mintableSupply only happens if mintableSupply > 0. But then mintableSupply is decremented. Therefore, if the admin mints exactly mintableSupply tokens, the new value for mintableSupply will be zero. After this, subsequent calls to mint can mint unlimited tokens.

Tools Used

VSCode

Recommended Mitigation Steps

There's a few ways around this, so I'll just recommend one: use a separate variable to keep track of the initial max supply. This could simply be a bool but using a uint also allows people to query what the init value was:

diff --git a/contracts/token/VariableSupplyERC20Token.sol b/contracts/token/VariableSupplyERC20Token.sol
index 1297a14..6d2a381 100644
--- a/contracts/token/VariableSupplyERC20Token.sol
+++ b/contracts/token/VariableSupplyERC20Token.sol
@@ -9,6 +9,7 @@ import "../AccessProtected.sol";
  */
 contract VariableSupplyERC20Token is ERC20, AccessProtected {
     uint256 public mintableSupply;
+    uint256 public initialMintableSupply;
 
     /**
     @notice A ERC20 token contract that allows minting at will, with limited or unlimited supply. No burning possible
@@ -26,6 +27,7 @@ contract VariableSupplyERC20Token is ERC20, AccessProtected {
         // Should we allow this?
         require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");
         mintableSupply = maxSupply_;
+        initialMintableSupply = maxSupply_;
         
         // Note: the check whether initial supply is less than or equal than mintableSupply will happen in mint fn.
         if(initialSupply_ > 0) {
@@ -36,8 +38,8 @@ contract VariableSupplyERC20Token is ERC20, AccessProtected {
     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) {
+        // initialMintableSupply = 0 means mint at will
+        if(initialMintableSupply > 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;
@@ -47,4 +49,4 @@ contract VariableSupplyERC20Token is ERC20, AccessProtected {
 
     // We can't really have burn, because that could make our vesting contract not work.
     // Example: if the user can burn tokens already assigned to vesting schedules, it could be unable to pay its obligations.
-}
\ No newline at end of file
+}

Summary

I've reviewed all of the Solidity code in scope, and found what I believe are two Medium severity issues that I've reported separately. Other than that, I think the code looks clean, and I appreciate the minimal design for the vesting contract. Here I'll just present some non-critical suggestions that I think could help improve the code's readability and maintainability.

Non-critical findings

Using uint112 for amounts

Is there a strong reason for using the unusual type uint112? If it's only for gas, I believe using the native type for the EVM could be safer and more convenient in the long run.

Keep in mind different tokens might want to use various values for the token decimals, so having the largest possible number type could give founders more flexibility when designing the token.

Changing everything to uint256 does produce a 10-20% gas increase in some functions, so I understand if you prefer to keep it this way. Even using uint128 could make things a bit safer and more flexible, and this barely introduces a gas increase compared to uint112. In any case I'd recommend documenting the reason for this decision so that it's clearer when looking at the code.

Name of the Claimed event

https://github.com/code-423n4/2022-09-vtvl/blob/26dda235d38d0f870c1741c9f7eef03229172bbe/contracts/VTVLVesting.sol#L64

With the word Claim being used for the main struct, I would suggest a more specific name, like ClaimTokensWithdrawn.

Use of the same revert message

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

I'd recommend using a different revert message for each case, to make debugging easier for users, e.g.

        Claim storage _claim = claims[_recipient];
        require(_claim.startTimestamp > 0, "NO_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, "CLAIM_NOT_ACTIVE");

Non-explicit imports

https://github.com/code-423n4/2022-09-vtvl/blob/26dda235d38d0f870c1741c9f7eef03229172bbe/contracts/VTVLVesting.sol#L5-L9

I recommend changing these to import { Foo } from ".../Foo.sol" to import exactly what's needed from each file. This helps avoid accidental name collisions.

Complex require logic

https://github.com/code-423n4/2022-09-vtvl/blob/26dda235d38d0f870c1741c9f7eef03229172bbe/contracts/VTVLVesting.sol#L270-L278

This is a bit hard to read. It could be unfolded to something more explicit like:

if (_cliffReleaseTimestamp > 0) {
    require(_cliffAmount > 0 && _cliffReleaseTimestamp <= _startTimestamp, "INVALID_CLIFF");
} else {
    require(_cliffAmount == 0, "INVALID_CLIFF");
}

Or even more verbose:

if (_cliffReleaseTimestamp > 0) {
    require(_cliffAmount > 0, "INVALID_CLIFF_AMOUNT_ZERO");
    require(_cliffReleaseTimestamp <= _startTimestamp, "INVALID_CLIFF_RELEASE_GT_START");
} else {
    require(_cliffAmount == 0, "INVALID_CLIFF_AMOUNT_NONZERO");
}

Unused Ownable import

https://github.com/code-423n4/2022-09-vtvl/blob/26dda235d38d0f870c1741c9f7eef03229172bbe/contracts/AccessProtected.sol#L5

The AccessProtected contract is not Ownable. Did you mean to add an owner role? If not, I'd suggest removing the unused import to prevent confusion.

Inaccurate reason for not allowing burning

https://github.com/code-423n4/2022-09-vtvl/blob/26dda235d38d0f870c1741c9f7eef03229172bbe/contracts/token/VariableSupplyERC20Token.sol#L48-L49

The VariableSupplyERC20Token comments specify that being able to burn tokens could break the vesting contracts. If only the owner of certain tokens can burn them, there is no way to burn any tokens on the vesting contract without first withdrawing them. Therefore, I think it'd be safe to make the token burnable.

Setting the first admin to msg.sender

https://github.com/code-423n4/2022-09-vtvl/blob/26dda235d38d0f870c1741c9f7eef03229172bbe/contracts/AccessProtected.sol#L16-L19

I think this sets some unnecessary restrictions to how this contract can be deployed - for composability, it might be desirable to e.g. deploy this contract from another contract, in which case setting the admin to msg.sender will make the contract inoperable. Or a user might want to use a deployer account that is separate from the account used to administer the funds, in which case two more transactions are needed to add the new admin and remove the deployer.

Making this more flexible only adds a bit more gas on the deployment, so I'd recommend changing the constructor to allow specifying a separate address for the first admin:

    constructor(address initialAdmin) {
        require(initialAdmin != address(0), "!ADMIN");
        _admins[initialAdmin] = true;
        emit AdminAccessSet(initialAdmin, true);
    }

(You'd then need to modify any contracts that inherit this to call the constructor accordingly)

Or if you still want to default to msg.sender, you can allow passing a zero address to initialAdmin:

    constructor(address initialAdmin) {
        if (initialAdmin == address(0)) {
            initialAdmin = _msgSender();
        }
        _admins[initialAdmin] = true;
        emit AdminAccessSet(initialAdmin, true);
    }