VTVL contest - Czar102's results

Lines of code

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

Vulnerability details

Impact

The admin of the token is not constrained to minting maxSupply_, they can mint any number of tokens.

Proof of Concept

// If we're using maxSupply, we need to make sure we respect it
// mintableSupply = 0 means mint at will
if(mintableSupply > 0) {
	require(amount <= mintableSupply, "INVALID_AMOUNT");
	// We need to reduce the amount only if we're using the limit, if not just leave it be
	mintableSupply -= amount;
}

The logic is as follows: if the amount that can be minted is zero, treat this as an infinite mint. Else require that the minted amount is not larger than mintable supply.

One can note that it is possible to mint all mintable supply. Then the mintable supply will be 0 which will be interpreted as infinity and any number of tokens will be possible to be minted.

Tools Used

Manual analysis

Recommended Mitigation Steps

Treat 2 ** 256 - 1 as infinity instead of 0.

Lines of code

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

Vulnerability details

Impact

If the token is reentrant, an admin can steal all tokens locked in the VTVLVesting contract while having active locks.

In other words, due to this exploit possibility, the contract may be insolvent with respect to active vestings. Note that revoking claim doesn't break this invariant since the vesting is closed in that case.

Proof of Concept

The reentrancy in the vested token can be used by an admin if the execution can be hijacked before the balance change occurs.

/**
@notice Admin withdrawal of the unallocated tokens.
@param _amountRequested - the amount that we want to withdraw
	*/
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);
}

Let's consider function withdrawAdmin. Firstly, the balance is checked and then if there is enough token surplus to withdraw, the withdrawal is allowed. The surplus is based on two values: numTokensReservedForVesting which isn't changed by this function and the balance of the contract.

If the owner hijacks the execution before the balance change in the token transfer (which is possible in, for example, ERC777), an admin can call this function again and it will allow for an invocation of another transfer since the token balance hasn't changed yet.

For example, if there is $1m in vestings in the contract, an admin can send $100k to it in tokens and recursively invoke withdrawalAdmin with the amount of $100k eleven times so that the whole contract balance will be drained.

Tools Used

Manual analysis

Recommended Mitigation Steps

Add ReentrancyGuard's nonReentrant to the withdrawAdmin function.