Unlock Protocol contest - loop's results

Handle

loop

Vulnerability details

When upgrading a lock the implementation is taken from _publicLockImpls[version]. The upgraded implementation first has to be added with the function addLockTemplate(address impl, uint16 version), which puts the new implementation at index version of the _publicLockImpls mapping.

When calling upgradeLock there is no check whether this has actually happened for the version the lock manager tries to update to. Assuming the user is lock manager, has given the correct version and the proxyAdmin is set, it is possible for the lock template at the version index to not be set yet. This will result in the lock being upgraded to the zero address and likely no longer be usable until upgraded to the correct template.

Proof of Concept

Upgrade process: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L249-L251

Adding a lock template: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L187-L193

Recommended Mitigation Steps

Add something along the lines of require(_publicLockImpls[version] != address(0), "Template not found").

Handle

loop

Vulnerability details

The function tokenOfOwnerByIndex has a requirement for _index to be 0. This is different from the specification, which states:

/// @dev Throws if `_index` >= `balanceOf(_keyOwner)` or if
///  `_keyOwner` is the zero address, representing invalid NFTs.

It also seems to be a bit unnecessary as _index is not even used in the following call getTokenIdFor(_keyOwner).

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinERC721Enumerable.sol#L44-L59

Recommended Mitigation Steps

Change require statement to something according to specification: require(_index < balanceOf(_keyOwner) && _keyOwner != address(0));

Handle

loop

Vulnerability details

The function updateKeyPricing in MixinLockCore.sol and _initializeMixinFunds in MixinFunds.sol have an invalid token check which can be moved to the start of the function. If an invalid token is given as input it will revert before trying to perform the action(s) which are currently done before the check. Since the check does not depend on these actions being completed it will save some gas if an invalid token is given as input if the check is done first.

Proof of Concept

• https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinFunds.sol#L24-L33
• https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinLockCore.sol#L169-L186

Recommended Mitigation Steps

Move reuire statements to start of the function.

Handle

loop

Vulnerability details

Some functions that are currently set to public are not called internally and can be external to save some gas when invoking these functions.

Proof of Concept

• https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L141-L151
• https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L153-L156
• https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L187-L193
• https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L211-L230
• https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L237-L260
• https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L289-L366
• https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L374-L384

Tools Used

Slither

Handle

loop

Vulnerability details

If recordKeyPurchase is invoked with an ERC-20 token which does not have a supported uniswap oracle maxTokens will perform a division by 0 if grossNetworkProduct has not been increased yet. This is due to valueInEth not being set if no supported oracle has been found, as the if condition is not met. As a result grossNetworkProduct will also not be increased in grossNetworkProduct = grossNetworkProduct + valueInETH;. Assuming the udtOracle has been set and _referrer is not address(0), the computation of maxtokens can be a division by 0 as grossNetworkProduct has not increased.

Alternatively, if grossNetworkProduct has been increased before, but there is no supported oracle, a lot of unnecessary computation is done because maxTokens will always be 0 due to valueInEth being 0. As a result, tokensToDistribute will also be 0 and nothing really happens except for a lot of computation, and thus gas cost.

Changing the if(address(oracle) != address(0)) to a require statement will revert the function if no supported oracle is found resulting in no 0 division or unnecessary computation.

Proof of Concept

Check for supported oracle: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L302-L304

Increase of gNP: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L311

Computation of maxTokens: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L328-L337

If maxTokens < tokensToDistribute: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L340-L345

Recommended Mitigation Steps

Change if(address(oracle) != address(0)) to something along the lines of require(address(oracle) != address(0), "No supported oracle");