VTVL contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

As per the comment in _createClaimUnchecked function, product team wanted _startTimestamp to be from past as well. But there is no limit on how much it could be in the past which means it could be last 100 years as well. In case it is too much in past, it would be possible to make claim for about 99% of amount at just starting of contract with no way for Admin to rectify the mistake (if user claims fast)

Proof of Concept

1. Admin calls createClaim function and by mistake passes _startTimestamp as block.timestamp-100 years and _endTimestamp as block.timestamp+1month

function createClaim(
            address _recipient, 
            uint40 _startTimestamp, 
            uint40 _endTimestamp, 
            uint40 _cliffReleaseTimestamp, 
            uint40 _releaseIntervalSecs, 
            uint112 _linearVestAmount, 
            uint112 _cliffAmount
                ) external onlyAdmin {
        _createClaimUnchecked(_recipient, _startTimestamp, _endTimestamp, _cliffReleaseTimestamp, _releaseIntervalSecs, _linearVestAmount, _cliffAmount);
    }

2. This internally calls _createClaimUnchecked where checks on _startTimestamp is performed. These are basic checks with no max limit on past date of _startTimestamp as shown below

function _createClaimUnchecked(
            address _recipient, 
            uint40 _startTimestamp, 
            uint40 _endTimestamp, 
            uint40 _cliffReleaseTimestamp, 
            uint40 _releaseIntervalSecs, 
            uint112 _linearVestAmount, 
            uint112 _cliffAmount
                ) private  hasNoClaim(_recipient) {
...
require(_startTimestamp > 0, "INVALID_START_TIMESTAMP");
 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");
Claim memory _claim = Claim({
            startTimestamp: _startTimestamp,
            endTimestamp: _endTimestamp,
            cliffReleaseTimestamp: _cliffReleaseTimestamp,
            releaseIntervalSecs: _releaseIntervalSecs,
            cliffAmount: _cliffAmount,
            linearVestAmount: _linearVestAmount,
            amountWithdrawn: 0,
            isActive: true
        });
...
}

3. As we can see here the _startTimestamp we provided simply pass all test case and a new claim gets created

4. Since _startTimestamp is 100 years in past from current date and _endTimestamp is block.timestamp+1month, so once this claim is made claim recipient would be able to make a claim of 99% already since as per contract 100 years have already passed on a vested duration of 100 years 1month

_currentTimestamp-_startTimestamp = block.timestamp-block.timestamp+100 years = 100 years

Recommended Mitigation Steps

Make a limit on how back _startTimestamp could be. Like allowing claim to be maximum 1 year past current time

Lines of code

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

Vulnerability details

Impact

Admin can withdraw locked funds

Proof of Concept

1. User A did some dev work for a protocol and protocol decided to pay the User using the vesting contract
2. Admin creates the claim for User A with Amount X for duration of 1 month
3. One month passes and User A has not claimed the amount yet.
4. Admin wants the steal the allotted amount back from dev but revokeClaim does not allow so
5. Admin can use a bypass by using Reentrancy attack in withdrawAdmin function

function withdrawAdmin(uint112 _amountRequested) public onlyAdmin {    
        // Allow the owner to withdraw any balance not currently tied up in contracts.
        uint256 amountRemaining = tokenAddress.balanceOf(address(this)) - numTokensReservedForVesting;

        require(amountRemaining >= _amountRequested, "INSUFFICIENT_BALANCE");

        // Actually withdraw the tokens
        // Reentrancy note - this operation doesn't touch any of the internal vars, simply transfers
        // Also following Checks-effects-interactions pattern
        tokenAddress.safeTransfer(_msgSender(), _amountRequested);

        // Let the withdrawal known to everyone
        emit AdminWithdrawn(_msgSender(), _amountRequested);
    }

6. Lets say below is status of contract

tokenAddress.balanceOf(address(this)) = 400
numTokensReservedForVesting = 300
amountRemaining = 400-300 = 100

7. So Admin can withdraw 100 amount securing the 300 amount of User A

8. Below code tries to transfer 100 amount to Admin contract

tokenAddress.safeTransfer(_msgSender(), _amountRequested);

9. Assume tokenAddress is a custom ERC20 contract which makes one event call to recipient contract before transferring any amount.

10. Once call is made to Admin contract, it reenters the withdrawAdmin function again. (Remember transfer has not completed yet). Since no state variables has changed so while reentering the state remains same

tokenAddress.balanceOf(address(this)) = 400
numTokensReservedForVesting = 300
amountRemaining = 400-300 = 100

11. Again in this reentered state transfer call is made which is success and 100 amount is transferred to Admin

tokenAddress.safeTransfer(_msgSender(), _amountRequested);

12. Now main call is executed which transfers amount 100 again making net transfer of 200 to Admin instead of 100

Recommended Mitigation Steps

Mark the withdrawAdmin function with nonReentrant modifier (openzepplin ReentrancyGuard)

Incorrect condition may prohibit valid scenario

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

Issue: In the constructor of VariableSupplyERC20Token.sol, it is always possible to have both initialSupply_ and maxSupply_ equal to zero (Admin allows unlimited minting with initial 0 tokens). However this is not possible due to below check

require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");

Recommendation: Remove the below check

require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");

Unrequired check wasting gas

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

Issue: In hasActiveClaim function, there is no need to check require(_claim.startTimestamp > 0, "NO_ACTIVE_CLAIM"); since it is more than enough to check _claim.isActive==true in hasActiveClaim function

Recommendation: Modify the hasActiveClaim function to eliminate the below require condition

require(_claim.startTimestamp > 0, "NO_ACTIVE_CLAIM");