Lines of code

Vulnerability details

Impact

allocatedTokens 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 they should get or even unable to make a claim withdrawal.

Proof of Concept

Suppose that the token balance is unfortunately less than the numTokensReservedForVesting. And then Admin calls withdrawAdmin().

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

Since the operation is not using safemath, so the substraction wont revert, and turns the amountRemaining into the max number of uint256. Then, the checks for amountRemaining >= _amountRequested would passed.

And now the token balance and numTokensReservedForVesting will have a very big difference where numTokensReservedForVesting has a much larger value than the token balance.

For the some withdraw after that will be no problems. But when the token balance is lower than the amount to withdraw, there will be a problem. The unlucky user will not able to withdraw his claim, because the function will revert because of the fail transfer.

    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 greater than numTokensReservedForVesting.

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

👎 1 indijanc reacted with thumbs down emoji

All reactions

• 👎 1 reaction