VTVL contest - OptimismSec's results

Lines of code

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

Vulnerability details

Impact

An admin can mint as many tokens as they want, even if maxSupply_ is set to a non-zero value in the constructor. The impact is that more tokens can be minted than users thought would get minted. Further impacts are:

• this dilutes the user's share of the underlying token when they thought it was undilutable
• if the vesting token is listed on an exchange an admin could sell the new tokens for a large financial gain
• this could break code that depends on the token's totalSupply

We are rating this as a high since unlimited dilution of a token is equivalent to reducing its value to almost zero.

Proof of Concept

1. Call constructor with a maxSupply_ greater than zero. The value of mintableSupply is will be set to a value greater than zero on line 28

       │ File: VariableSupplyERC20Token.sol #28

  27   │         require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");
  28   │         mintableSupply = maxSupply_;
  29   │
  30   │         // Note: the check whether initial supply is less than or equal than mintableSupply will happen in mint fn.
  31   │         if(initialSupply_ > 0) {
  32   │             mint(_msgSender(), initialSupply_);
  33   │         }

2. Call mint function takes in two parameters (the address to be minted and amount to be minted.)

The if-statement that controls the supply (line 40-46) of the token can be skipped if mintableSupply == 0.

An admin only needs to call mint with an amount equal to the the current mintableSupply variable.

Now that mintableSupply == 0 any subsequent calls to mint will skip the if-statement and the control flow always moves to line 45. The admin can mint endlessly, at will.

A hardhat test that demonstrates the bug is:

import { expect } from "chai";
import { ethers } from "hardhat";

describe("VariableERC20TokenTest", async () => {
	it("Test unlimited VariableERC20 mint", async () => {
		const [adminOne] = await ethers.getSigners();
		const originalMintableSupply = 10_000_000;
		const initialSupply = 1_000_000;
		const VariableSupplyERC20Fac = await ethers.getContractFactory(
			"VariableSupplyERC20Token"
		);
		//
		const VariableERC20 = await VariableSupplyERC20Fac.connect(adminOne).deploy(
			"AToken",
			"ATK",
			initialSupply,
			originalMintableSupply
		);
		// exploits
		// can mint after making the current mintableAmount the amount to be minted(originalMintableSupply - initialSupply)
		await VariableERC20.connect(adminOne).mint(
			adminOne.address,
			originalMintableSupply - initialSupply
		);

		console.log(VariableERC20.mintableSupply());
		//mintabeSupply equals to zero and can mint unlimited afterwards
		expect(await VariableERC20?.mintableSupply()).to.equal(0);
		await VariableERC20.connect(adminOne).mint(
			adminOne.address,
			10_000_000_000_000
		);
	});
});

Recommend Mitigation Steps

Add an extra state variable that shows and records the number of tokens minted add a require statement that reverts if the numberMinted + amount > mintableSupply. Also, make mintableSupply and immutable variable that represents the maximum supply.

diff --git a/contracts/token/VariableSupplyERC20Token.sol b/contracts/token/VariableSupplyERC20Token.sol
index 1297a14..f514394 100644
--- a/contracts/token/VariableSupplyERC20Token.sol
+++ b/contracts/token/VariableSupplyERC20Token.sol
@@ -8,7 +8,8 @@ import "../AccessProtected.sol";
 @notice A ERC20 token contract that allows minting at will, with limited or unlimited supply.
  */
 contract VariableSupplyERC20Token is ERC20, AccessProtected {
-    uint256 public mintableSupply;
+    uint256 public immutable mintableSupply;
+    uint256 public numberMinted;
 
     /**
     @notice A ERC20 token contract that allows minting at will, with limited or unlimited supply. No burning possible
@@ -30,6 +31,7 @@ contract VariableSupplyERC20Token is ERC20, AccessProtected {
         // Note: the check whether initial supply is less than or equal than mintableSupply will happen in mint fn.
         if(initialSupply_ > 0) {
             mint(_msgSender(), initialSupply_);
+            numberMinted += initialSupply_;
         }
     }
 
@@ -38,9 +40,8 @@ contract VariableSupplyERC20Token is ERC20, AccessProtected {
         // 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;
+            require(numberMinted + amount <= mintableSupply, "INVALID_AMOUNT");
+            numberMinted += amount;
         }
         _mint(account, amount);
     }

QA Report

Remarks/Recommendations

It would probably be a good idea for there to be a contract owner that can never be removed and has more privileges than the admins, so that functionality can be restored if it's deemed necessary.

Coding style improvements

VTVLVesting.sol:L111

require(_claim.isActive == true, "NO_ACTIVE_CLAIM");

The == true is redundant

2. There should be a remove or unsetAdmin function for more code clarity (using the same name setAdmin to remove admins)

AccessProtected.sol#L39

Documentation improvements

1. On line 370

// Conversion of timestamp to uint40 should be safe since 48 bit allows for a lot of years.

should be changed to since 40 bits allows for

More documentation is required for the "zero admin" case. Discussion with the sponsor confirmed that the case of their being zero admins is supported. However, there should be more documentation provided for this feature. In particular users should be informed that they will not be able to call many of the functions of the contract should all admins be removed.

Gas Report

Use memory instead using of storage for storing the claims struct to some save gas

• Affected lines of code
  • L106:

  Claim storage _claim = claims[_recipient];

  • L124

  Claim storage _claim = claims[_recipient];

  • L197

  Claim storage _claim = claims[_recipient];

  • L207

  Claim storage _claim = claims[_recipient];

  • L216

  Claim storage _claim = claims[_recipient];

  • L367

  Claim storage usrClaim = claims[_msgSender()];

  • L420

  Claim storage _claim = claims[_recipient];

Use unchecked blocks and ++i to make loops more efficient

• L353
• like so

      │ File: contracts/VTVLVesting.sol
353 + │         for (uint256 i = 0; i < length;) {
354   │             _createClaimUnchecked(_recipients[i], _startTimestamps[i], _endTimestamps[i], _cliffRelease
      │ Timestamps[i], _releaseIntervalsSecs[i], _linearVestAmounts[i], _cliffAmounts[i]);
  +   |             unchecked {
  +   |                  ++i;
  +   |             }
  +   |
355   │         }
356   │ 

The function withdrawAdmin() is only declared and not used anywhere else in the contract, make it external

See L398.

Some calculations won't reasonably overflow

Use unchecked blocks for them to save gas.

The function is internal and the parameters are not controlled by the user, so it is safe to use unchecked.

L166-L179 can be changed as follows:

──────┬────────────────────────────────────────────────────────────────────────────────────────────────────────
      │          File: contracts/VTVLVesting.sol#L166-L179

166   │         if(_referenceTs > _claim.startTimestamp) {
    + |             unchecked {
167   │                 uint40 currentVestingDurationSecs = _referenceTs - _claim.startTimestamp; // How long since the start
168   │                 // Next, we need to calculated the duration truncated to nearest releaseIntervalSecs
169   │                 uint40 truncatedCurrentVestingDurationSecs = (currentVestingDurationSecs / _claim.releaseIntervalSecs) * _claim.releaseIntervalSecs;
170   │                 uint40 finalVestingDurationSecs = _claim.endTimestamp - _claim.startTimestamp; // length of the interval
171   │ 
172   │                 // Calculate the linear vested amount - fraction_of_interval_completed * linearVestedAmount
173   │                 // Since fraction_of_interval_completed is truncatedCurrentVestingDurationSecs / finalV estingDurationSecs, the formula becomes
174   │                 // truncatedCurrentVestingDurationSecs / finalVestingDurationSecs * linearVestAmount, so we can rewrite as below to avoid 
175   │                 // rounding errors
176   │                 uint112 linearVestAmount = _claim.linearVestAmount * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs;
177   │ 
178   │                 // Having calculated the linearVestAmount, simply add it to the vested amount
179   │                 vestAmt += linearVestAmount;
   +  |              }
            }