VTVL contest - Chom's results

Lines of code

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

Vulnerability details

Impact

The claim can't be recreated for the same address after a claim has been revoked. Revoking a claim can be used in a condition where you need to alter the claim parameters. But once you have revoked it, you can't recreate the claim to alter the parameters anymore.

Our vesting contract is deliberately designed to allow admin revocation in the circumstances of early employment termination (before the end of vesting period specified).

But once that employee rejoins, they are forced to change their wallet address which is not a good design.

Proof of Concept

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

revokeClaim set isActive to false

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

Creating a claim requires hasNoClaim

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

hasNoClaim only checks for startTimestamp. Revoking claim not resetting startTimestamp, so this check failed. As a result, the claim can't be recreated for the same address after a claim has been revoked.

Recommended Mitigation Steps

hasNoClaim should check for only isActive instead of startTimestamp

    modifier hasNoClaim(address _recipient) {
        require(_claim.isActive == false, "CLAIM_ALREADY_EXISTS");
        _;
    }

if _linearVestAmount == 0, _startTimestamp can't be same as _endTimestamp

You have noted that "if _linearVestAmount == 0, should we perhaps force that start and end ts are the same?" but it contradicts with this check

require(_startTimestamp < _endTimestamp, "INVALID_END_TIMESTAMP"); // _endTimestamp must be after _startTimestamp

if (_startTimestamp == _endTimestamp) it will revert with "INVALID_END_TIMESTAMP"

To fix this you should

require(_startTimestamp < _endTimestamp || (_linearVestAmount == 0 && _startTimestamp ==_endTimestamp) , "INVALID_END_TIMESTAMP"); // _endTimestamp must be after _startTimestamp

Add unchecked

    function withdraw() hasActiveClaim(_msgSender()) external {
        ...

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

        unchecked {
        // 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;
        }

        ...
    }

allowance > usrClaim.amountWithdrawn already check for over/underflow and amountRemaining should never overflow / underflow numTokensReservedForVesting or amountWithdrawn (allowance - usrClaim.amountWithdrawn + usrClaim.amountWithdrawn = allowance)

Add unchecked

    function _baseVestedAmount(Claim memory _claim, uint40 _referenceTs) internal pure returns (uint112) {
            ...

            // 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, truncatedCurrentVestingDurationSecs, finalVestingDurationSecs;

                unchecked {
                currentVestingDurationSecs = _referenceTs - _claim.startTimestamp; // How long since the start
                // Next, we need to calculated the duration truncated to nearest releaseIntervalSecs
                truncatedCurrentVestingDurationSecs = (currentVestingDurationSecs / _claim.releaseIntervalSecs) * _claim.releaseIntervalSecs;
                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;
            }
        }

       ...
    }

Use custom errors instead of require

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

Should be rewrite as

error NOTHING_TO_WITHDRAW();
...
if(allowance <= usrClaim.amountWithdrawn) revert NOTHING_TO_WITHDRAW();

Unchecked { ++i }

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

Should be rewrite as

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