Unlock Protocol contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

function addLockTemplate has a comment that it adds a new template:

  Registers a new PublicLock template immplementation

However, it does not validate impl and version, so an owner can override an existing version/implementation. Another problem is that it doesn't check that versions are in incremental order, thus gaps may occur if an owner (accidentally) skips a version. Lock upgrades can only happen in an incremental order though.

Recommended Mitigation Steps

My suggestion is to get rid of the version parameter, and use/increment publicLockLatestVersion instead. Also, consider validating that impl is not empty.

Handle

pauliax

Vulnerability details

Impact

function _cancelAndRefund is not protected from re-entrancy. _cancelAndRefund uses an unsafe _transfer that performs a low-level call which is susceptible to re-entrancy attack. These calls should be the last step or you may receive unexpected guests. Even the comment says that but it is actually not the last step:

    if (refund > 0) {
      // Security: doing this last to avoid re-entrancy concerns
      _transfer(tokenAddress, _keyOwner, refund);
    }
    // inform the hook if there is one registered
    if(address(onKeyCancelHook) != address(0))
    {
      onKeyCancelHook.onKeyCancel(msg.sender, _keyOwner, refund);
    }

Recommended Mitigation Steps

Consider either making this _transfer the last step or adding re-entrancy protection: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol

Handle

pauliax

Vulnerability details

Impact

function recordKeyPurchas does not distribute the tokens on chainId > 1 when the balance is not sufficient:

  uint balance = IMintableERC20(udt).balanceOf(address(this));
  if (balance > tokensToDistribute) {
    // Only distribute if there are enough tokens
    IMintableERC20(udt).transfer(_referrer, tokensToDistribute - devReward);
    IMintableERC20(udt).transfer(owner(), devReward);
  }

I see at least 2 problems here. First, the check should be inclusive >=, because now when balance = tokensToDistribute, it will still skip the distribution. Another problem, I think it is not fair when only a small fraction is missing, you should consider distributing the whole balance in this case.

Recommended Mitigation Steps

Here is my prosposed solution:

  uint balance = IMintableERC20(udt).balanceOf(address(this));
  if (balance >= tokensToDistribute) {
    IMintableERC20(udt).transfer(_referrer, tokensToDistribute - devReward);
    IMintableERC20(udt).transfer(owner(), devReward);
  } else {
    devReward = balance * 20 / 100;
    IMintableERC20(udt).transfer(_referrer, balance - devReward);
    IMintableERC20(udt).transfer(owner(), devReward);
  }

Handle

pauliax

Vulnerability details

Impact

function purchase is payable, thus it should validate that msg.value is 0 when tokenAddress != address(0) to prevent accidental sent Ether.

Recommended Mitigation Steps

Check no ether was sent when the token is not a native currency.

Handle

pauliax

Vulnerability details

Impact

function _assignNewTokenId first increments _totalSupply and then assigns token id, so ids start from 1, not 0. However, function tokenByIndex in MixinERC721Enumerable expects the index to be less than totalSupply:

  /// @notice Enumerate valid NFTs
  /// @dev Throws if `_index` >= `totalSupply()`.
  /// @param _index A counter less than `totalSupply()`
  /// @return The token identifier for the `_index`th NFT,
  ///  (sort order not specified)
  function tokenByIndex(
    uint256 _index
  ) public view
    returns (uint256)
  {
    require(_index < _totalSupply, 'OUT_OF_RANGE');
    return _index;
  }

This mismatch between indexes and token ids may trick other platforms or integrations.

Recommended Mitigation Steps

I think the solution is simply returning index + 1:

    require(_index < _totalSupply, 'OUT_OF_RANGE');
    return _index + 1; // index 0 = token id 1

Handle

pauliax

Vulnerability details

Impact

Discrepancies between the interface and implementation, e.g. IUnlock declares a function createLock with 6 parameters but Unlock has only 1 data parameter.

Recommended Mitigation Steps

Consider explicitly inheriting an interface to avoid such discrepancies:

  contract Unlock is IUnlock
  // similar with other contracts

Handle

pauliax

Vulnerability details

Impact

After purchasing the key, it calls the hook:

  onKeyPurchaseHook.onKeyPurchase(msg.sender, _recipient, _referrer, _data, inMemoryKeyPrice, pricePaid);

It passes inMemoryKeyPrice as the amount paid, however, the documentation above this function says:

   * @param minKeyPrice the price including any discount granted from calling this
   * hook's `keyPurchasePrice` function

so it expects an amount with a discount included, but actually receives inMemoryKeyPrice which has already discount deducted from it.

Recommended Mitigation Steps

Consider either passing inMemoryKeyPrice + discount, or updating the comment.

Handle

pauliax

Vulnerability details

Impact

There are some validations missing that could make the system more robust. For instance:

function updateRefundPenalty could validate:

  _freeTrialLength <= expirationDuration
  _refundPenaltyBasisPoints <= BASIS_POINTS_DEN

function shareKey could validate that _to != keyOwner and _timeShared > 0.

function grantKeys could validate that the length of _recipients, _expirationTimestamps, and _keyManagers are the same.

function grantKeys may also have onlyIfAlive and notSoldOut modifiers applied. refunds could also consider onlyIfAlive unless that's intended behavior.

Also, I think the same address shouldn't be able to purchase a key again if he refunded a key from the same lock before.

function upgradeLock should validate that version <= publicLockLatestVersion to prevent an upgrade to a non yet existent version.

Recommended Mitigation Steps

Consider applying the aforementioned validations.

Handle

pauliax

Vulnerability details

Impact

The unchecked keyword can be applied in the following lines of code since there are statements before that ensure the arithmetic operations would not cause an integer underflow or overflow:

  if (refund > penalty) {
    refund -= penalty;

  if(discount < minKeyPrice)
  {
    minKeyPrice -= discount;

  require(unlockDiscount <= minKeyPrice, 'INVALID_DISCOUNT_FROM_UNLOCK');
  minKeyPrice -= unlockDiscount;
     
  for (uint i = 0; i < 20; i++)
     
  // Math: safeSub is not required since `hasValidKey` confirms timeRemaining is positive
  uint timeRemaining = key.expirationTimestamp - block.timestamp;

There might be more places where this can be applied.

Handle

pauliax

Vulnerability details

Impact

Some expressions do not change, so can be precalculated to reduce gas usage. For example, this:

  uint devReward = tokensToDistribute * 20 / 100;

can be simplified to:

  uint devReward = tokensToDistribute / 5;

and this:

  (125 * 10 ** 18) / 100

doesn't need to be reevaluated every time, so can be extracted as a constant.

Handle

pauliax

Vulnerability details

Impact

function _assignNewTokenId does not necessarily need to check if (_key.tokenId == 0) as this check was already performed in all places where this function is called. Similarly, function getTransferFee does not need to check getHasValidKey as I don't see a case where this was not checked before.

Handle

pauliax

Vulnerability details

Impact

This:

    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;
      }
    }

can be refactored to avoid duplicate checks:

    if (freeTrialLength != 0 && timeRemaining + freeTrialLength >= expirationDuration) {
      refund = keyPrice;
    } else {
      refund = keyPrice * timeRemaining / expirationDuration;

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

Handle

pauliax

Vulnerability details

Impact

The check could be inclusive <= to handle the case when timePlusFee = timeRemaining:

  if(timePlusFee < timeRemaining)

Handle

pauliax

Vulnerability details

Impact

Assigned operations to constant variables are re-evaluated every time. See https://github.com/ethereum/solidity/issues/9232 Change to 'constant' to 'immutable':

  bytes32 private constant _TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
  bytes32 private constant _DELEGATION_TYPEHASH =
        keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)");
  bytes32 public constant LOCK_MANAGER_ROLE = keccak256("LOCK_MANAGER");
  bytes32 public constant KEY_GRANTER_ROLE = keccak256("KEY_GRANTER");

Handle

pauliax

Vulnerability details

Impact

valueInETH can be 0 when the token is not native but oracle is not set for it. Consider skipping all the expensive calculations in recordKeyPurchase to save gas when valueInETH is 0. Also, in special cases, this can invoke a division by zero runtime error when calculating maxTokens and dividing by grossNetworkProduct.

Handle

pauliax

Vulnerability details

Impact

Incrementing / decrementing values is cheapest by doing variable++/--; Examples where this can be applied:

  counter._value += 1;
  counter._value = value - 1;
  k = k-1;

Handle

pauliax

Vulnerability details

Impact

address(this).address2Str() can be extracted as a constant to avoid reevaluation again each time tokenURI function is called.