VTVL contest - zzzitron's results

Lines of code

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

Vulnerability details

Impact

Admin can mint more than the intended max supply

Proof of Concept

In the VariableSupplyERC20Token, if non-zero maxSupply_ is passed in the constructor, it is intended to cap the total supply. And the state variable mintableSupply is set to be maxSupply_ (line 28) to keep track of the remaining amount to mint. As tokens are minted the mintableSupply would be reduced (line 43).

On the other hand, if the maxSupply_ is zero, there is no cap for the total supply and the admin can mint at will.

The problem happens when the maxSupply_ was not zero, and all of intended supply was minted. In that case the mintableSupply would be reduced to zero. After that point, if the admin calls mint function again, it should fail, because all intended supply was minted. However, with the current implementation, it will simply skip the check (line 40), because the mintableSupply is now zero.

// VariableSupplyERC20Token

// constructor
 27         require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");
 28         mintableSupply = maxSupply_;

// mint
 36     function mint(address account, uint256 amount) public onlyAdmin {
 37         require(account != address(0), "INVALID_ADDRESS");
 38         // If we're using maxSupply, we need to make sure we respect it
 39         // mintableSupply = 0 means mint at will
 40         if(mintableSupply > 0) {
 41             require(amount <= mintableSupply, "INVALID_AMOUNT");
 42             // We need to reduce the amount only if we're using the limit, if not just leave it be
 43             mintableSupply -= amount;
 44         }
 45         _mint(account, amount);
 46     }

Tools Used

None

Recommended Mitigation Steps

Save the maxSupply_ and compare the (token's total supply + to be minted amount) to it. It also saves gas, as there is no more need to update the mintableSupply additionally to the total supply, which is done anyway by the openzeppelin's ERC20.

<!-- zzzitron H00 -->

Lines of code

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

Vulnerability details

Impact

Some users with claim may not be able to withdraw, if the balance of the VTVLVesting has changed

Proof of Concept

The balanceOf the contract VTVLVesting should be greater or equal to the numTokensReservedForVesting, and it is ensured in the withdrawAdmin and createClaim. However, there are tokens with changing balances without transferring tokens (ref). If the balance of VTVLVesting should be reduced below the numTokensReservedForVesting, there will be users with valid claim, who cannot withdraw their claims.

Tools Used

None

Recommended Mitigation Steps

It should be clearly stated what kind of tokens are allowed to be used in the VTVLVesting.

<!-- zzzitron M01 -->

Lines of code

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

Vulnerability details

Impact

It is possible to renounce every admin

Proof of Concept

In the below function setAdmin, there is no safe guard to have at least one admin. It is possible to set the only admin to be false, therefore there is no more admin remaining. The information for admins is stored in a mapping, thus it is hard to keep track of how many admins are enabled. So there can be an accident to set the last admin to be disenabled. If it happens there can be no longer any admin for the contract.

In the case to renounce all the admin, it is advised to have a separate function to prevent from renouncing all admins unintentionally.

// AccessProtected
// setAdmin
 39     function setAdmin(address admin, bool isEnabled) public onlyAdmin {
 40         require(admin != address(0), "INVALID_ADDRESS");
 41         _admins[admin] = isEnabled;
 42         emit AdminAccessSet(admin, isEnabled);
 43     }

Tools Used

None

Recommended Mitigation Steps

For setAdmin function, ensure there is at least one admin available. For example, do not allow the msg.sender to disenable itself. If it is intended to be able to renounce all the admins, add a separate function for that.

<!-- zzzitron M00 -->