VTVL contest - sorrynotsorry's results

Lines of code

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

Vulnerability details

Impact

VTVLVesting.sol has _createClaimUnchecked function to create the claims internally while validating parameters with the users' allocations. However, _releaseIntervalSecs is not validated comparing to user's _linearVestAmount and _startTimestamp _endTimestamp. Theoratically, _linearVestAmount should be equal to ((_endTimestamp - _startTimestamp) * _releaseIntervalSecs) so the _releaseIntervalSecs = _linearVestAmount / ((_endTimestamp - _startTimestamp) But this check was never done.

If the _releaseIntervalSecs is validated either to a higher or to a lower amount, it will create unfair distributions amongst the users during withdrawals due to being higher/lower than it should be. And also it may end up with the last withdrawals can be reverted due to the calculation board not matching.

Proof of Concept

    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");
        // Do we need to check whether _startTimestamp is greater than the current block.timestamp? 
        // Or do we allow schedules that started in the past? 
        // -> Conclusion: we want to allow this, for founders that might have forgotten to add some users, or to avoid issues with transactions not going through because of discoordination between block.timestamp and sender's local time
        // require(_endTimestamp > 0, "_endTimestamp must be valid"); // not necessary because of the next condition (transitively)
        require(_startTimestamp < _endTimestamp, "INVALID_END_TIMESTAMP"); // _endTimestamp must be after _startTimestamp
        require(_releaseIntervalSecs > 0, "INVALID_RELEASE_INTERVAL");
        require((_endTimestamp - _startTimestamp) % _releaseIntervalSecs == 0, "INVALID_INTERVAL_LENGTH");


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


        // No point in allowing cliff TS without the cliff amount or vice versa.
        // Both or neither of _cliffReleaseTimestamp and _cliffAmount must be set. If cliff is set, _cliffReleaseTimestamp must be before or at the _startTimestamp
        require( 
            (
                _cliffReleaseTimestamp > 0 && 
                _cliffAmount > 0 && 
                _cliffReleaseTimestamp <= _startTimestamp
            ) || (
                _cliffReleaseTimestamp == 0 && 
                _cliffAmount == 0
        ), "INVALID_CLIFF");


        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
    }

Permalink

Tools Used

Manual Review

Recommended Mitigation Steps

The _releaseIntervalSecs should be validated comparing to user's _linearVestAmount and _startTimestamp _endTimestamp.

QA (LOW & NON-CRITICAL)

[L-01] Consider making contracts Pausable

The contracts are bringing many risks including how the token distribution is carried out. Making the contracts pausable would at least prevent malicious actions to be carried out in all projects using VTVL contracts if any zero-day vulnerability occurs.

[L-02] There should be an admins array

AccessPortected.sol has setAdmin function that can add or remove admins for the protocol. Since there would be lots of projects using these contracts, it would be ideal to implement a require statement to check the number of admins, so that if the admins.length = 1, that admin should not be removed by accident. If the last admin is removed accidentally, the whole vesting contract would be in an irreversible position.

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

[L-03] Check of code existence

VTVLVesting.sol's constructor sets the token address to be vested for. However, there is no code existence checked for the token in order to prevent any mistaken input.

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

Permalink

[L-04] Logic error at _baseVestedAmount function

The expression should be as below;

            if(_referenceTs >= _claim.endTimestamp) {
                _referenceTs = _claim.endTimestamp;
            }

rather than;

            if(_referenceTs > _claim.endTimestamp) {
                _referenceTs = _claim.endTimestamp;
            }

Permalink

[L-05] linearVestAmount data type

VTVLVesting's linearVestAmount variable is of uint112 type. However, it can't answer the needs if the underlying token has an enormous supply greater than this amount