Unlock Protocol contest - HardlyDifficult's results

Handle

HardlyDifficult

Vulnerability details

Impact

Save 4.5k gas on createLock.

Proof of Concept

Replace https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L223:

locks[newLock] = LockBalances({
      deployed: true, totalSales: 0, yieldedDiscountTokens: 0
    });

with: locks[newLock].deployed = true;

Tools Used

yarn test and diff gas report

Recommended Mitigation Steps

Remove any instance explicitly assigning new variables to 0 - only do so when purposefully overwriting data that may or may not already be there.

There is no concept of null or undefined in Solidity, everything is 0 if it has not otherwise been defined. When writing to storage, there is a non-trivial cost to write 0 even though it's effectively a no-op.

Handle

HardlyDifficult

Vulnerability details

Impact

Saves 2k gas on upgradeLock

Proof of Concept

Rename proxyAdmin https://github.com/code-423n4/2021-11-unlock/blob/52f3f3d0524dda28aea327181c3479d85782007b/smart-contracts/contracts/Unlock.sol#L99 to __gap_was_proxyAdmin and replace references to use ProxyAdmin(proxyAdminAddress) instead.

In the upgradeLock function, both proxyAdminAddress and proxyAdmin are read. There is a fairly high gas cost to read from storage, so using the same variable casted instead saves gas.

Tools Used

yarn test and diff the gas report.

Recommended Mitigation Steps

Remove proxyAdmin since it was a private variable, and use the public proxyAdminAddress instead. Be sure to put a gap variable in that slot so upgrades are not broken.

Handle

HardlyDifficult

Vulnerability details

Impact

Save up to 45k gas on purchase key (27k gas on average per the test suite). grantKeys, shareKey, and safeTransferFrom have similar savings.

Proof of Concept

Remove this line https://github.com/code-423n4/2021-11-unlock/blob/52f3f3d0524dda28aea327181c3479d85782007b/smart-contracts/contracts/mixins/MixinKeys.sol#L50 and replace with a gap variable instead.

Tools Used

yarn test and diff gas report

Recommended Mitigation Steps

Replace address[] public owners with a gap so to preserve upgrade compatibility.

There is a high cost to push into an array. Arrays are generally not useful for other contracts because they will not be able to walk entries once there's a non-trivial number of owners. Off-chain, indexers such as The Graph are better suited to track data like this.

If there is an on-chain need for this, it may be better to be explicit about the value and maybe optimize the data for what's required -- e.g. maybe it's a way to check if an address currently or has previously owned a key, that could be handy on-chain but this array does not allow a contract to determine that.

Handle

HardlyDifficult

Vulnerability details

Impact

Save 1.7k gas on a key purchase when there's no discount, and 3.2k gas when discounts are added. A minor improvement, but this also seems easier to follow in trace logs or with a tool like Tenderly.

Proof of Concept

In the PublicLock purchase remove the calls to _purchasePriceFor and recordKeyPurchase and replace it with uint inMemoryKeyPrice = _purchase(_recipient, _referrer, _data); which was implemented as:

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

    return unlockProtocol.recordKeyPurchaseAndGetDiscount(_recipient, minKeyPrice, _referrer);
  }

Then in Unlock, add a new function:

  function recordKeyPurchaseAndGetDiscount(
    address recipient,
    uint minKeyPrice,
    address referrer
  ) public returns (uint inMemoryKeyPrice) {
    if(minKeyPrice > 0)
    {
      (uint unlockDiscount, uint unlockTokens) = computeAvailableDiscountFor(recipient, minKeyPrice);
      inMemoryKeyPrice = minKeyPrice;
      if (unlockDiscount > 0)
      {
        // INVALID_DISCOUNT_FROM_UNLOCK error assumed from safe math
        inMemoryKeyPrice -= unlockDiscount;
        recordConsumedDiscount(unlockDiscount, unlockTokens);
      }
    }

    // Record price without any tips
    recordKeyPurchase(inMemoryKeyPrice, referrer);
  }

This approach clones the current behavior, but moves logic into Unlock instead of going back and forth between PublicLock and Unlock.

Tools Used

yarn test and diff the gas reporter table.

Recommended Mitigation Steps