Lines of code
<https://github.com/code-423n4/2022-09-vtvl/blob/main/contracts/VTVLVesting.sol#L364-L392&gt;

Vulnerability details

Impact

Allocated tokens can get messed up when the amountRemaining in the withdrawAdmin() function is underflowed in rare cases. This will make ‘numTokensReservedForVesting’ will have a larger amount of funds compared to the funds in the token. This will make it possible for the user not to get a claim amount that is less than the original or even unable to make a claim withdrawal.

Proof of Concept

Suppose that unfortunately token balance has lower balance than numTokensReservedForVesting. And then admin wants to withdraw the token, so he will call withdrawAdmin() function. withdrawAdmin() will not allow to withdraw the allocated token (token that used to cliff vested and linear vested). Since numTokenReservedForVesting is greater than the token balance, the amountRemaining will underflow. But the function wouldn’t revert, because the operation is not using safemath. So the operation will success and return the max number of uint112. Since the amountRemaining is greater than amountRequested, check will passed.

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

Given :
Token Balance = 10000
numTokensReservedForVesting = 10050
amountRequired = 5000

After the withdraw from admin, the remaining balance in the token is 5000, and now token balance and numTokensVestedAmount will have a very big difference where numTokensVestedAmount has a much larger value. For some withdrawals after this will be no problems, but the unlucky user would not able to withdraw his claim, because the withdraw will revert due to the failed transfer inside the withdraw() function (not enough balance to transfer the user claim).

    function withdraw() hasActiveClaim(_msgSender()) external {
        // Get the message sender claim - if any

        Claim storage usrClaim = claims[_msgSender()];

        // 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.
        uint112 allowance = vestedAmount(_msgSender(), uint40(block.timestamp));

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

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

        // Let withdrawal known to everyone.
        emit Claimed(_msgSender(), amountRemaining);
    }

Recommended Mitigation Steps

Use safemath for amountRemaining in withdrawAdmin() function or add check for token balance is larger than numTokensReservedForVesting.

The text was updated successfully, but these errors were encountered:

All reactions