Unlock Protocol contest - WatchPug's results

Handle

WatchPug

Vulnerability details

It isn’t safe to simply add a state variable when upgrading contracts because it "shifts down" all of the state variables below in the inheritance chain. This makes the storage layouts incompatible.

Therefore, Mixin*** contracts should include an empty reserved space in storage, usually named __gap.

See: https://docs.openzeppelin.com/contracts/3.x/upgradeable#storage_gaps

Handle

WatchPug

Vulnerability details

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

For example:

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinPurchase.sol#L130-L152

  function _purchasePriceFor(
    address _recipient,
    address _referrer,
    bytes memory _data
  ) internal view
    returns (uint minKeyPrice, uint unlockDiscount, uint unlockTokens)
  {
    if(address(onKeyPurchaseHook) != address(0))
    {
      minKeyPrice = onKeyPurchaseHook.keyPurchasePrice(msg.sender, _recipient, _referrer, _data);
    }
    else
    {
      minKeyPrice = keyPrice;
    }

    if(minKeyPrice > 0)
    {
      (unlockDiscount, unlockTokens) = unlockProtocol.computeAvailableDiscountFor(_recipient, minKeyPrice);
      require(unlockDiscount <= minKeyPrice, 'INVALID_DISCOUNT_FROM_UNLOCK');
      minKeyPrice -= unlockDiscount;
    }
  }

minKeyPrice -= unlockDiscount will never underflow.

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinRefunds.sol#L133-L159

  function _getCancelAndRefundValue(
    address _keyOwner
  )
    private view
    hasValidKey(_keyOwner)
    returns (uint refund)
  {
    Key storage key = keyByOwner[_keyOwner];
    // Math: safeSub is not required since `hasValidKey` confirms timeRemaining is positive
    uint timeRemaining = key.expirationTimestamp - block.timestamp;
    if(timeRemaining + freeTrialLength >= expirationDuration) {
      refund = keyPrice;
    } else {
      refund = keyPrice * timeRemaining / expirationDuration;
    }

    // Apply the penalty if this is not a free trial
    if(freeTrialLength == 0 || timeRemaining + freeTrialLength < expirationDuration)
    {
      uint penalty = keyPrice * refundPenaltyBasisPoints / BASIS_POINTS_DEN;
      if (refund > penalty) {
        refund -= penalty;
      } else {
        refund = 0;
      }
    }
  }

key.expirationTimestamp - block.timestamp and refund -= penalty will never underflow.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/Unlock.sol#L211-L230

  function createLock(
    bytes memory data
  ) public returns(address)
  {
    require(proxyAdminAddress != address(0), "proxyAdmin is not set");
    require(publicLockAddress != address(0), 'MISSING_LOCK_TEMPLATE');

    // deploy a proxy pointing to impl
    TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy(publicLockAddress, proxyAdminAddress, data);
    address payable newLock = payable(address(proxy));

    // assign the new Lock
    locks[newLock] = LockBalances({
      deployed: true, totalSales: 0, yieldedDiscountTokens: 0
    });

    // trigger event
    emit NewLock(msg.sender, newLock);
    return newLock;
  }

The payable keyword on newLock is redundant, removing it can reduce contract size and save gas at deployment.

Handle

WatchPug

Vulnerability details

Every reason string takes at least 32 bytes.

Use short reason strings that fits in 32 bytes or it will become more expensive.

Instances include:

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/Unlock.sol#L241-L241

require(_isLockManager(lockAddress, msg.sender) == true, "caller is not a manager of this lock");

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/Unlock.sol#L246-L246

require( version == currentVersion + 1, 'version error: only +1 increments are allowed');

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinRoles.sol#L40-L40

require( hasRole(LOCK_MANAGER_ROLE, msg.sender), 'MixinRoles: caller does not have the LockManager role');

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinRoles.sol#L45-L45

require(isKeyGranter(msg.sender) || isLockManager(msg.sender), 'MixinRoles: caller does not have the KeyGranter or LockManager role');

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/UnlockDiscountTokenV2.sol#L27-L27

require(_EIP712NameHash() == bytes32(0), "Already initialized: EIP712_init, ERC20Permit_init, ERC20Votes_init");

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/UnlockUtils.sol#L21-L47

  function uint2Str(
    uint _i
  ) internal pure
    returns (string memory _uintAsString)
  {
    // make a copy of the param to avoid security/no-assign-params error
    uint c = _i;
    if (_i == 0) {
      return '0';
    }
    uint j = _i;
    uint len;
    while (j != 0) {
      len++;
      j /= 10;
    }
    bytes memory bstr = new bytes(len);
    uint k = len;
    while (c != 0) {
        k = k-1;
        uint8 temp = (48 + uint8(c - c / 10 * 10));
        bytes1 b1 = bytes1(temp);
        bstr[k] = b1;
        c /= 10;
    }
    return string(bstr);
  }

Recommendation

Change to:

  function uint2Str(
    uint _i
  ) internal pure
    returns (string memory _uintAsString)
  {
    // make a copy of the param to avoid security/no-assign-params error
    uint c = _i;
    if (_i == 0) {
      return '0';
    }
    uint j = _i;
    uint len;
    while (j != 0) {
      len++;
      j /= 10;
    }
    bytes memory bstr = new bytes(len);
    uint k = len;
    while (c != 0) {
        bstr[--k] = bytes1(uint8(48 + c % 10));
        c /= 10;
    }
    return string(bstr);
  }

Handle

WatchPug

Vulnerability details

Unused named returns increase contract size and gas usage at deployment.

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/Unlock.sol#L257-L260

function _isLockManager(address lockAddress, address _sender) private view returns(bool isManager) {
  IPublicLock lock = IPublicLock(lockAddress);
  return lock.isLockManager(_sender);
}

isManager is unused.

Recommendation

Change to:

function _isLockManager(address lockAddress, address _sender) private view returns(bool) {
  IPublicLock lock = IPublicLock(lockAddress);
  return lock.isLockManager(_sender);
}

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinLockCore.sol#L164-L186

  /**
   * A function which lets the owner of the lock change the pricing for future purchases.
   * This consists of 2 parts: The token address and the price in the given token.
   * In order to set the token to ETH, use 0 for the token Address.
   */
  function updateKeyPricing(
    uint _keyPrice,
    address _tokenAddress
  )
    external
    onlyLockManager
    onlyIfAlive
  {
    uint oldKeyPrice = keyPrice;
    address oldTokenAddress = tokenAddress;
    require(
      _tokenAddress == address(0) || IERC20Upgradeable(_tokenAddress).totalSupply() > 0,
      'INVALID_TOKEN'
    );
    keyPrice = _keyPrice;
    tokenAddress = _tokenAddress;
    emit PricingChanged(oldKeyPrice, keyPrice, oldTokenAddress, tokenAddress);
  }

Check of _tokenAddress at L180 can be done earlier to avoid unnecessary code execution at L177-178 and save some gas for failure transactions.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinPurchase.sol#L92-L98

uint pricePaid;
if(tokenAddress != address(0))
{
    pricePaid = _value;
    IERC20Upgradeable token = IERC20Upgradeable(tokenAddress);
    token.transferFrom(msg.sender, address(this), pricePaid);
}

token is unnecessary as it's being used only once. Can be changed to:

uint pricePaid;
if(tokenAddress != address(0))
{
    pricePaid = _value;
    IERC20Upgradeable(tokenAddress).transferFrom(msg.sender, address(this), pricePaid);
}

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/Unlock.sol#L257-L260

  function _isLockManager(address lockAddress, address _sender) private view returns(bool isManager) {
    IPublicLock lock = IPublicLock(lockAddress);
    return lock.isLockManager(_sender);
  }

lock is unnecessary as it's being used only once. Can be changed to:

  function _isLockManager(address lockAddress, address _sender) private view returns(bool isManager) {
    return IPublicLock(lockAddress).isLockManager(_sender);
  }

Handle

WatchPug

Vulnerability details

By the time MixinRoles._initializeMixinRoles() is called, there are no roles configured yet, therefore the check of !isLockManager(sender) and !isKeyGranter(sender) is redundant.

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinRoles.sol#L22-L36

  function _initializeMixinRoles(address sender) internal {

    // for admin mamangers to add other lock admins
    _setRoleAdmin(LOCK_MANAGER_ROLE, LOCK_MANAGER_ROLE);

    // for lock managers to add/remove key granters
    _setRoleAdmin(KEY_GRANTER_ROLE, LOCK_MANAGER_ROLE);

    if (!isLockManager(sender)) {
      _setupRole(LOCK_MANAGER_ROLE, sender);  
    }
    if (!isKeyGranter(sender)) {
      _setupRole(KEY_GRANTER_ROLE, sender);
    }
  }

Recommendation

Change to:

  function _initializeMixinRoles(address sender) internal {

    // for admin mamangers to add other lock admins
    _setRoleAdmin(LOCK_MANAGER_ROLE, LOCK_MANAGER_ROLE);

    // for lock managers to add/remove key granters
    _setRoleAdmin(KEY_GRANTER_ROLE, LOCK_MANAGER_ROLE);

    _setupRole(LOCK_MANAGER_ROLE, sender);

    _setupRole(KEY_GRANTER_ROLE, sender);
  }

Handle

WatchPug

Vulnerability details

For public functions, the input parameters are copied to memory automatically which costs gas. If a function is only called externally, making its visibility as external will save gas because external functions’ parameters are not copied into memory and are instead read from calldata directly.

For example:

• Unlock.sol#createLock()
• UnlockProtocolTimelock.sol#initialize()

Handle

WatchPug

Vulnerability details

For the storage variables that will be accessed multiple times, cache and read from the stack can save ~100 gas from each extra read (SLOAD after Berlin).

In Unlock.sol#_deployProxyAdmin(), _proxyAdmin can be cached to avoid unnecessary storage reads.

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/Unlock.sol#L162-L166

  function _deployProxyAdmin() private returns(address) {
    proxyAdmin = new ProxyAdmin();
    proxyAdminAddress = address(proxyAdmin);
    return address(proxyAdmin);
  }

Recommendation

Change to:

  function _deployProxyAdmin() private returns(address) {
    ProxyAdmin _proxyAdmin = new ProxyAdmin();
    proxyAdmin = _proxyAdmin;
    proxyAdminAddress = address(_proxyAdmin);
    return address(_proxyAdmin);
  }

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/Unlock.sol#L237-L260

  function upgradeLock(address payable lockAddress, uint16 version) public returns(address) {
    require(proxyAdminAddress != address(0), "proxyAdmin is not set");

    // check perms
    require(_isLockManager(lockAddress, msg.sender) == true, "caller is not a manager of this lock");

    // check version
    IPublicLock lock = IPublicLock(lockAddress);
    uint16 currentVersion = lock.publicLockVersion();
    require( version == currentVersion + 1, 'version error: only +1 increments are allowed');

    // make our upgrade
    address impl = _publicLockImpls[version];
    TransparentUpgradeableProxy proxy = TransparentUpgradeableProxy(lockAddress);
    proxyAdmin.upgrade(proxy, impl);

    emit LockUpgraded(lockAddress, version);
    return lockAddress;
  }

  function _isLockManager(address lockAddress, address _sender) private view returns(bool isManager) {
    IPublicLock lock = IPublicLock(lockAddress);
    return lock.isLockManager(_sender);
  }

_isLockManager() is unnecessary as it's being used only once. Can be changed to:

  function upgradeLock(address payable lockAddress, uint16 version) public returns(address) {
    require(proxyAdminAddress != address(0), "proxyAdmin is not set");

    // check perms
    require(IPublicLock(lockAddress).isLockManager(msg.sender), "caller is not a manager of this lock");

    // check version
    IPublicLock lock = IPublicLock(lockAddress);
    uint16 currentVersion = lock.publicLockVersion();
    require( version == currentVersion + 1, 'version error: only +1 increments are allowed');

    // make our upgrade
    address impl = _publicLockImpls[version];
    TransparentUpgradeableProxy proxy = TransparentUpgradeableProxy(lockAddress);
    proxyAdmin.upgrade(proxy, impl);

    emit LockUpgraded(lockAddress, version);
    return lockAddress;
  }

Handle

WatchPug

Vulnerability details

For the storage variables that will be accessed multiple times, cache and read from the stack can save ~100 gas from each extra read (SLOAD after Berlin).

In MixinRefunds.sol#_getCancelAndRefundValue: freeTrialLength, expirationDuration and keyPrice are accessed multiple times.

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinRefunds.sol#L133-L159

  function _getCancelAndRefundValue(
    address _keyOwner
  )
    private view
    hasValidKey(_keyOwner)
    returns (uint refund)
  {
    Key storage key = keyByOwner[_keyOwner];
    // Math: safeSub is not required since `hasValidKey` confirms timeRemaining is positive
    uint timeRemaining = key.expirationTimestamp - block.timestamp;
    if(timeRemaining + freeTrialLength >= expirationDuration) {
      refund = keyPrice;
    } else {
      refund = keyPrice * timeRemaining / expirationDuration;
    }

    // Apply the penalty if this is not a free trial
    if(freeTrialLength == 0 || timeRemaining + freeTrialLength < expirationDuration)
    {
      uint penalty = keyPrice * refundPenaltyBasisPoints / BASIS_POINTS_DEN;
      if (refund > penalty) {
        refund -= penalty;
      } else {
        refund = 0;
      }
    }
  }

Recommendation

Change to:

  function _getCancelAndRefundValue(
    address _keyOwner
  )
    private view
    hasValidKey(_keyOwner)
    returns (uint refund)
  {
    Key storage key = keyByOwner[_keyOwner];
    // Math: safeSub is not required since `hasValidKey` confirms timeRemaining is positive
    uint timeRemaining = key.expirationTimestamp - block.timestamp;
    uint _expirationDuration = expirationDuration;
    uint _freeTrialLength = freeTrialLength;
    uint _keyPrice = keyPrice;
    if(timeRemaining + _freeTrialLength >= _expirationDuration) {
      refund = _keyPrice;
    } else {
      refund = _keyPrice * timeRemaining / _expirationDuration;
    }

    // Apply the penalty if this is not a free trial
    if(_freeTrialLength == 0 || timeRemaining + _freeTrialLength < _expirationDuration)
    {
      uint penalty = _keyPrice * refundPenaltyBasisPoints / BASIS_POINTS_DEN;
      if (refund > penalty) {
        refund -= penalty;
      } else {
        refund = 0;
      }
    }
  }

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinRefunds.sol#L142-L147

uint timeRemaining = key.expirationTimestamp - block.timestamp;
if(timeRemaining + freeTrialLength >= expirationDuration) {
    refund = keyPrice;
} else {
    refund = keyPrice * timeRemaining / expirationDuration;
}

PoC

Given:

• keyPrice is 1 ETH;
• expirationDuration is 1 year.

1. Alice calls purchase() 10 times, paid 1 ETH and got 1 extra year each time;
2. Alice calls cancelAndRefund(), expected to receive a refund of 10 ETH but only receive 1 ETH instead.

Impact

Users may suffer fund loss when they purchase multiple times and refund.

Recommendation

Consider recording the total amount paid by the user, and refund accordingly.