Unlock Protocol contest - 0x0x0x's results

Handle

0x0x0x

Vulnerability details

Proof of Concept

Since this function doesn't check whether there is an implementation for the given version number. Implementations can be updated by calling this function, which is different from the description. Moreover, updating can create an attack vector and i don't recommend this behaviour. Also it would be nice, if it checks whether publicLockLatestVersion is incremented by one or not.

Tools Used

Manual analysis

Recommended Mitigation Steps

Rename or add checks to make sure implementations doesn't get updated

Handle

0x0x0x

Vulnerability details

Impact

All ERC20 token does not support approving directly and require first approving for an amount of 0. By using safeApprove or approving 0 first, the function will have better compatability with different tokens.

Tools Used

Manual analysis

Handle

0x0x0x

Vulnerability details

Proof of Concept

setVotingDelay, setVotingPeriod and setQuorum have a similar function structure. All can emit first and then change to avoid caching. Current implementation of setVotingDelay is as follows:

function setVotingDelay(uint256 newVotingDelay) public onlyGovernance {
  uint256 oldVotingDelay = _votingDelay;
  _votingDelay = newVotingDelay;
  emit VotingDelayUpdated(oldVotingDelay, newVotingDelay);
}

This can be replaced with

function setVotingDelay(uint256 newVotingDelay) public onlyGovernance {
  emit VotingDelayUpdated(_votingDelay, newVotingDelay);
  _votingDelay = newVotingDelay;
}

to save gas and have a shorter code.

Tools Used

Manual

Handle

0x0x0x

Vulnerability details

Proof of Concept

Example:

for (uint i = 0; i < arr.length; i++) {
//Operations not effecting the length of the array.
}

Loading length of array costs gas. Therefore, the length should be cached, if the length of the array doesn't change inside the loop. Recommended implementation:

uint length = arr.length;
for (uint i = 0; i < length; i++) {
//Operations not effecting the length of the array.
}

By doing so the length is only loaded once rather than loading it as many times as iterations (Therefore, less gas is spent).

Occurences

./contracts/mixins/MixinGrantKeys.sol:29:    for(uint i = 0; i < _recipients.length; i++) {

Handle

0x0x0x

Vulnerability details

Proof of Concept

It is better to put require statements as early as possible to avoid wasting gas.

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

can be replaced with following code block

      address recipient = _recipients[i];
      require(recipient != address(0), 'INVALID_ADDRESS');
      
      uint expirationTimestamp = _expirationTimestamps[i];
      Key storage toKey = keyByOwner[recipient];
      require(expirationTimestamp > toKey.expirationTimestamp, 'ALREADY_OWNS_KEY');
      
      address keyManager = _keyManagers[i];

Tools Used

Manual

Handle

0x0x0x

Vulnerability details

Proof of Concept

As mentioned in the comment this line is safe to execute. by using unchecked gas can be saved. https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/Unlock.sol#L313

Tools Used

manual

Handle

0x0x0x

Vulnerability details

Proof of Concept

lock, impl, currentVersion and proxy are used only once, but all of them are cached. To save gas, all of them can be directly passed.

Handle

0x0x0x

Vulnerability details

Proof of Concept

Current implementation converts addresses the bytes32, does skip empty bytes and read where the address is. To do so + 12 is used to loop over the address. By directly converting addresses to bytes20, gas can be saved and more gas will be saved by avoiding + 12. By doing so the function consumes around %10 less gas.

function address2Str(
    address _addr
  ) internal pure
    returns(string memory)
  {
    bytes32 value = bytes20(_addr);
    bytes memory alphabet = '0123456789abcdef';
    bytes memory str = new bytes(42);
    str[0] = '0';
    str[1] = 'x';
    for (uint i = 0; i < 20; i++) {
      str[2+i*2] = alphabet[uint8(value[i] >> 4)];
      str[3+i*2] = alphabet[uint8(value[i] & 0x0f)];
    }
    return string(str);
  }

Tools Used

Manual analysis

Handle

0x0x0x

Vulnerability details

Proof of Concept

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/Unlock.sol#L329-L362 can be implemented cheaper, with several upgrades as follows:

          if (chainId > 1)
          {
            // non mainnet: we distribute tokens using asymptotic curve between 0 and 0.5
            // maxTokens = IMintableERC20(udt).balanceOf(address(this)).mul((valueInETH / grossNetworkProduct) / (2 + 2 * valueInETH / grossNetworkProduct));
            uint balance = IMintableERC20(udt).balanceOf(address(this));
            maxTokens = balance * valueInETH / (2 + 2 * valueInETH / grossNetworkProduct) / grossNetworkProduct;
            
            if(maxTokens > 0){
              if(tokensToDistribute > maxTokens){
                tokensToDistribute = maxTokens;
              }
            }
            if (balance > tokensToDistribute) {
              uint devReward = tokensToDistribute * 20 / 100;
              // Only distribute if there are enough tokens
              IMintableERC20(udt).transfer(_referrer, tokensToDistribute - devReward);
              IMintableERC20(udt).transfer(owner(), devReward);
            }

          } else {
            // Mainnet: we mint new token using log curve
            maxTokens = IMintableERC20(udt).totalSupply() * valueInETH / 2 / grossNetworkProduct;
            if(maxTokens > 0){
              if(tokensToDistribute > maxTokens){
                tokensToDistribute = maxTokens;
              }
            }
            uint devReward = tokensToDistribute * 20 / 100;
            IMintableERC20(udt).mint(_referrer, tokensToDistribute - devReward);
            IMintableERC20(udt).mint(owner(), devReward);
          }

This code block makes less computations, when maxTokens == 0. Moreover balance is cached and shared.

Tools Used

Manual