VTVL contest - Lambda's results

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/26dda235d38d0f870c1741c9f7eef03229172bbe/contracts/VTVLVesting.sol#L446

Vulnerability details

Impact

The system has measures in place to prevent the admin from withdrawing an amount that would result in a balance which is lower then the remaining tokens reserved for vesting (which should never happen according to the protocol documents: "They can also withdraw the remaining amount not allocated on claims back to their wallet."). However, the whole balance can be withdrawn using withdrawOtherTokens for some specific tokens. Namely, there are tokens that have multiple entrypoints, but the check in withdrawOtherToken only checks if the addresses are different. Note that these tokens are not some obscure tokens that nobody uses, Compound was almost hit by a very similar problem, because TrueUSD had two entrypoints.

Proof Of Concept

Let's say that some token with two addresses, address(A) and address(B) is used as the underlying token. tokenAddress is set to address(A). The admin can now call withdrawOtherToken(address(B)) and drain the whole contract balance. This does not even emit some special events, so it will be very hard for the end user to detect the reason for it.

Recommended Mitigation Steps

Check that the withdrawal does not decrease the balance of the underlying token.

Lines of code

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

Vulnerability details

Impact

The system has measures in place to prevent the admin from withdrawing an amount that would result in a balance which is lower then the remaining tokens reserved for vesting (which should never happen according to the protocol documents: "They can also withdraw the remaining amount not allocated on claims back to their wallet."). However, when an ERC777 token is used as the underlying token (which works without problems and probably will be done by users, as ERC777 is backwards compatible with ERC20), this check can be circumvented (see Proof Of Concept). This allows administrators to drain the whole contract and leads to a situation where tokenAddress.balanceOf(address(this)) < numTokensReservedForVesting.

Proof Of Concept

• A vesting contract with a ERC777 token as the underlying token is deployed. 1,000,000 tokens are transferred to the contract and 950,000 are currently allocated for vesting.
• In theory it should only possible for the admins to withdraw 50,000 tokens. However, this can be circumvented like this: Let's say that the admin is a smart contract (or rather the admin performs the administration over a smart contract that he specifically prepared).
  • This contract calls withdrawAdmin(50_000).
  • Before updating the state (which includes the balance of the account), the ERC777 token calls the tokensToSend hook on the smart contract. Now, the contract can call withdrawAdmin(50_000) within the hook again. Note that tokenAddress.balanceOf(address(this)) is still 1,000,000 (because the state was not yet updated), so the amountRemaining check succeeds.
  • This can be repeated 20 times (always within the hook), which drains the contract completely.

Note that this is not the only way to exploit ERC777 reentrancy. It could also be exploited by the admin in withdraw (when there is a claim for the admin) and instead of calling withdrawAdmin repeatedly, the admin could also call createClaim in the hook to create a claim that pushes numTokensReservedForVesting over the balance. This could be even harder for users to detect, because there would be no events for repeated withdrawals (only a claim creation, which is generally a normal operation, with the only difference that this claim should not have been allowed).

Recommended Mitigation Steps

Add a reentrancy guard to withdraw, withdrawAdmin, and createClaim.

Design / Architecture

• Currently, the cliff date cannot be after the vesting start date. In my experience, there are scenarios where companies have a cliff date during a vesting period. Such situations currently could not be handled by VTVL.
• It is not possible for the admin to increase a claim afterwards. I think that this could be limiting in practice. For instance, it can happen that the vesting details of an employee are changed after a promotion.
• After one claim has ended, it is not possible to create another one for the same address, which is also quite limiting in my opinion. Often times, a new vesting period is agreed upon a company and an employee when the previous has ended. Currently, the employee would need to generate another address for that, which is not very user friendly.

Technical Issues

• Using a uint112 for amounts (especially for numTokensReservedForVesting, which is the global sum) can be insufficient for certain tokens. Consider a token with 24 decimals and where one token has a value of 0.0000001 USD. The maximum amount for which the protocol can be used is 2^112 / 10^24 * 0.0000001 USD = 519.23 USD, which is extremely low.

• The check require(amount <= mintableSupply, "INVALID_AMOUNT"); in VariableSupplyERC20Token.mint is unnecessary, because the subtraction on the next line will underflow anyways if this is not true.
• Decrementing the variable mintableSupply in VariableSupplyERC20Token.mint is not necessary. The remaining mintable supply could always be calculated based on the initialSupply_ (if it would be stored), the maximum supply and the already minted tokens (standard ERC20 variable _totalSupply). The remaining supply is then maxSupply_ - _totalSupply + initialSupply_.
• In VTVLVesting.sol:353, the loop iteration can be marked as unchecked because an overflow is not possible (as the iterator is bounded) and i++ can be replaced with ++i.
• Custom errors could be used instead of string error reasons to save gas.