code-423n4 / 2022-09-vtvl-findings

0 stars 0 forks source link

No check of vesting completion can break the distribution when the admin withdraws tokens #456

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

VTVLVesting.sol has withdrawAdmin function to allow admins to withdraw the unallocated tokens. However, it's not controlled whether the vesting is completed. If an uncontrolled withdraw occurs in a FullPremintERC20Token contract, than it would break the distribution ledger to the vesting users.

Proof of Concept

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

Permalink

Tools Used

Manual Review

Recommended Mitigation Steps

Check that the vesting is completed for the underlying token.

indijanc commented 2 years ago

Looks invalid: numTokensReservedForVesting tracks all claims full allocated amount no matter the vesting timeline.

0xean commented 2 years ago

closing as invalid.