VTVL contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

In VTVLVesting._baseVestedAmount(), the funds might be locked inside the contract forever with uint112 overflow.

Currently, it doesn't consider uint112 overflow during multiply and it's very likely to happen when the vesting duration is not short like 1 year.

In this case, the funds for that claim will be locked inside the contract permanently.

Proof of Concept

As we can see here, the allowed token amount for uint112 is around 5e15 after ignoring 18 decimals.

But when the vest duration is 1 year here, truncatedCurrentVestingDurationSecs is 3600 * 24 * 365 = 31536000.

So if token amount is greater than 5e15 / 31536000 = 1.6e8, it will revert with uint112 overflow and below scenario would be possible.

• The Admin created a claim for a user A. linearVestAmount = 2e8, VestDuration(= endTimestamp - startTimestamp) = 2 years.
• After 1 year, A tried to withdraw the available amount but it's failed here.
• Admin can't revoke the claim also because it will revert also with this function because final verst duration is longer than 1 year.
• So the funds for this claim can't be withdrawn forever.

It will be easier to happen when the token decimals are longer than 18.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We should modify this calculation like below.

uint112 linearVestAmount = uint112(((uint256)_claim.linearVestAmount) * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs);

Lines of code

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

Vulnerability details

Impact

The reentrance is possible with VTVL.withdrawAdmin().

As we can see here, the contract should contain enough balance for users' active claims so that users can trust they get receive the shares if the claims aren't revoked by admin.

But the admin can withdraw all funds using VTVL.withdrawAdmin() and users can't receive as expected even though the claims are active.

Proof of Concept

As we can see here, the token for this contract can be any ERC20 token and we can consider it's possible to be an ERC777 token.

Also, we can assume the token contains a hook _beforeTokenTransfer() like this.

• The contract has 100 tokens, numTokensReservedForVesting = 90.
• The admin calls withdrawAdmin() using _amountRequested = 10.
• At the first time, this require() is true and transfer will be started.
• But before it updates the token balances of the contract, the admin receives a hook _beforeTokenTransfer() and he calls withdrawAdmin() again with the same param.
• Then tokenAddress.balanceOf(address(this)) will be still 100 because the balance is not updated yet and it will work properly.
• Admin continues recalling 10 times and he can withdraw all of 100 tokens and users can't get the shares with the active claims.
• When the admin revokes the claims using revokeClaim(), there is an opportunity that users can withdraw the available tokens by front-running but they wouldn't take care of withdrawAdmin() as it's designed to withdraw some free tokens.

Even though the admin can create/revoke claims as he wants, the above case should be fixed.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Recommend using the ReentrancyGuard library of Openzeppelin

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/AccessProtected.sol#L39

Vulnerability details

Impact

AccessProtected.setAdmin() can enable/disable admin access and msg.sender(active admin) can disable himself using this function.

So if admin disables himself by fault, admin roles will be lost forever.

Proof of Concept

I don't think the admin might disable himself when there is only one admin.

But below scenario would be possible.

• There are 2 admins A and B now and they discussed to remove one of them.
• So A removed his role by calling setAdmin(A, false).
• But B forgot which admin to remove and removed his account also at the same time.
• Then they won't have admin roles forever.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

I think it would be good to add more requirements for this function.

We can modify msg.sender can't disable himself like below.

function setAdmin(address admin, bool isEnabled) public onlyAdmin {
    require(admin != address(0), "INVALID_ADDRESS");

    if(!isEnabled) {
        require(admin != msg.sender, "can't disable");
    }

    _admins[admin] = isEnabled;
    emit AdminAccessSet(admin, isEnabled);
}