VTVL contest - pauliax's results

Lines of code

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

Vulnerability details

Impact

revokeClaim should send the user what is already vested at this moment before deactivating the claim. e.g. imagine a situation when a user has never claimed the tokens and has 90% already vested but suddenly the admin decides to revoke it and the user is left with nothing. I believe this is not fair from the users' perspective and they should receive already accrued tokens. Even more unlikely but the worse situation would be if admin keys were compromised and a malicious actor was able to revoke all the claims and then withdrawAdmin all the tokens.

I am submitting this as of Medium severity because it might incur users losing their legitimate tokens that were already claimable but not yet claimed.

Recommended Mitigation Steps

The protocol should respect users and regard their rewards. Already accrued tokens should be sent to the users before deactivating the claim.

Lines of code

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

Vulnerability details

Impact

When minting the tokens in VariableSupplyERC20Token the mintableSupply is reduced, thus you can bypass the max supply limit once it hits 0 because 0 means unlimited. As far as I understand, the total supply should never reach the cap set in the constructor (parameter maxSupply_).

Proof of Concept

  function mint(address account, uint256 amount) public onlyAdmin {
      require(account != address(0), "INVALID_ADDRESS");
      // 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;
      }
      _mint(account, amount);
  }

Steps:

1. Mint mintableSupply.
2. mintableSupply is reduced to 0.
3. Mint any amount.

Recommended Mitigation Steps

Recommendation: rename mintableSupply to maxSupply (0 meaning unlimited). Do not decrement it here and before performing the mint check that maxSupply == 0 || totalSupply() + amount <= maxSupply.

• _cliffReleaseTimestamp > 0 is not really needed here:

  require( 
    _cliffReleaseTimestamp > 0 && 
    _cliffAmount > 0 && 
    _cliffReleaseTimestamp <= _startTimestamp
  )

because _startTimestamp was validated before:

  require(_startTimestamp > 0, "INVALID_START_TIMESTAMP");

• No need to initialize to default values:

  uint112 public numTokensReservedForVesting = 0;

• This is the first assignment to vestAmt:

  if(_referenceTs >= _claim.cliffReleaseTimestamp) {
      vestAmt += _claim.cliffAmount;
  }

can save a bit of gas by replacing += with just =.

• Can use unchecked maths here and everywhere else where overflow/underflow is not possible:

  if(_referenceTs > _claim.startTimestamp) {
      uint40 currentVestingDurationSecs = _referenceTs - _claim.startTimestamp;

• The duration is re-calculated every time the _baseVestedAmount is queried:

  uint40 finalVestingDurationSecs = _claim.endTimestamp - _claim.startTimestamp;

even though nor start, nor end timestamps do not change. Wouldn't it be better to store the duration in the Claim struct then?

• Repeated access of storage usrClaim.amountWithdrawn variable:

  require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW");
  ...
  uint112 amountRemaining = allowance - usrClaim.amountWithdrawn;

• Can just directly assign usrClaim.amountWithdrawn = allowance:

  uint112 allowance = vestedAmount(_msgSender(), uint40(block.timestamp));
  ...
  uint112 amountRemaining = allowance - usrClaim.amountWithdrawn;
  ...
  usrClaim.amountWithdrawn += amountRemaining;