Unlock Protocol contest - kenzo's results

Handle

kenzo

Vulnerability details

Not all ERC20 tokens revert on failed transfer. This is why using SafeERC20 is recommended. purchase does not use it.

Impact

If a key is using as payment an ERC20 token that does not throw on failed transfer, a user will be able to buy a key without paying, and even later claiming a refund, thereby stealing other users funds.

Proof of Concept

purchase method uses regular transferFrom: token.transferFrom(msg.sender, address(this), pricePaid); https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinPurchase.sol#L97 (Although IERC20Upgradeable is already using SafeERC20Upgradeable)

Recommended Mitigation Steps

Change the line to: token.safeTransferFrom(msg.sender, address(this), pricePaid);

Handle

kenzo

Vulnerability details

Unlock contains a feature in which a key buyer can ask for a refund. The refund is sent from the lock - where the purchase funds were sent. The lock manager can withdraw all funds from the lock. Therefore, if the lock manager withdraws enough profits from the lock, the user would not be able to cancel his key and request refund. Even if a lock manager is not malicious, if he would want to enable users to cancel their key, he would have to keep track of how much tokens need to be kept in the contract in order to enable this - not a trivial calculation. A naive lock manager might accidently disable refunds for his clients.

Impact

Refunds are not guaranteed. A user might buy a key expecting to cancel it within some time, only to discover he can not cancel it. (This loss of user funds if why I consider this a high risk finding.) An unaware lock manager who just wants to withdraw all his profits might accidently discover that he removed his users' ability to cancel their key.

Notes

It seems the Unlock team is aware to some extent that withdrawing breaks refunds, as they state in the withdraw function:

   * TODO: consider allowing anybody to trigger this as long as it goes to owner anyway?
   *  -- however be wary of draining funds as it breaks the `cancelAndRefund` and `expireAndRefundFor`
   * use cases.

However, even if just the owner is allowed to call it, he may break the refund functionality - on purpose or accidently. Looking on Unlock documentation I don't see a warning to creators about withdrawing their funds.

Proof of Concept

withdraw function has no limit on the amount withdrawn, therefore the owner can withdraw all funds: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinLockCore.sol#L133:#L162 cancelAndRefund transfers the funds from the same lock contract: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinRefunds.sol#L118 Therefore if there are not enough funds, the transfer will fail.

Recommended Mitigation Steps

Perhaps a sort of MasterChef-like shares system can be implemented in order to make sure the owner leaves enough funds in the lock to process refunds.

Handle

kenzo

Vulnerability details

Lock manager can change key pricing. The refund mechanism calculates refund according to current key price, not price actually paid.

Impact

A user refunding can get less (or more) funds than deserved.

Proof of Concept

Refund only takes the current price into account: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinRefunds.sol#L144:#L152 Lock manager can update key price at any point, and the old price is not saved anywhere: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinLockCore.sol#L183 So if for example a key price has gone down, a user who tried to refund will get less funds than deserved.

Recommended Mitigation Steps

Consider saving the amount the user paid, and refund according to that. Or having a kind of a price snapshot/version mechanism.

Handle

kenzo

Vulnerability details

If calling transferFrom with _from == _recipient, the key will get destroyed (meaning the key will be set as expired and set the owner's key to be 0).

Impact

A key manager or approved might accidently destroy user's token.

Note: this requires user error and so I'm not sure if this is a valid finding. However, few things make me think that it is valid:

• Unlock protocol checks for transfer to 0-address, so some input validation is there
• Since other entities other than the owner can be allowed to transfer owner's token, it might be best to make sure such accidental mistake could not happen.
• This scenario manifests a unique and probably unintended behavior

Proof of Concept

By following transferFrom's execution: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinTransfer.sol#L109:#L166 One can see that in the case where _from == _recipient with a valid key:

• The function will deduct transfer fee from the key
• The function will incorrectly add more time to the key's expiration (L151)
• The function will expire and reset the key (L155) Therefore, the user will lose his key without getting a refund.

Recommended Mitigation Steps

Add a require statement in the beginning of transferFrom: require(_from != _recipient, 'TRANSFER_TO_SELF');

Handle

kenzo

Vulnerability details

A malicious user can call shareKey repeatedly, transferring miniscule amounts of his key to different accounts, thereby minting new keys until maxNumberOfKeys is reached.

Impact

Malicious user can grief and make lock purchasing become disabled.

Proof of Concept

In shareKey, any user can mint a new token without paying anything extra: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinTransfer.sol#L83:#L84 And there's no minimum on _timeShared. Once _assignNewTokenId is called, _totalSupply is incremented. https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinKeys.sol#L311:#L322 Therefore a user can call shareKey repeatedely until a lock has been "sold out". Note: maxNumberOfKeys cannot be updated after lock deployment.

Recommended Mitigation Steps

I think it depends design/ux wise how you want to mitigate this. One option for example would be to not count these shared keys towards the maxNumberOfKeys. Note: a similar situation exists with grantKey, where the lock manager can grant keys which are free but still counted towards maxNumberOfKeys. But there I find it less of an issue as only the lock manager can issue these keys, so it's perhaps more acceptible.

Handle

kenzo

Vulnerability details

Unlock can add a new lock version, which lock owners can upgrade to, even before the implementation has been initialized. So anybody could initialize it before Unlock does.

Impact

If somebody else has initialized the new lock version before Unlock has called setLockTemplate - Unlock couldn't set this version as the default template as it has already been initialized. (Unlock would have to deploy another implementation.) If the new lock version has some mechanism of delegatecall or selfdestruct, the malicious actor could call it and brick the locks which have upgraded to that version.

Proof of Concept

When Unlock is adding a new lock version, there is no verification on whether it is initialized or not. https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L187:#L193 When upgrading a lock, there is no verification on whether it is initialized or not. https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L237:#L255 Only if the lock is being set as the default lock template, would it be initialized: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L423:#L438 But at that time it might have already been initialized by somebody else.

Recommended Mitigation Steps

I might be missing something, but I see no reason to not move the initialization of the lock to addLockTemplate. This will ensure that any lock implementation that is used by the protocol/users belongs to Unlock and not to another actor.

Handle

kenzo

Vulnerability details

More keys can be minted than maxNumberOfKeys since shareKey and grantKey do not check if the lock is sold out.

Impact

More keys can be minted than intended.

Proof of Concept

In both shareKey and grantKey, if minting a new token, a new token is simply minted (and _totalSupply increased) without checking it against maxNumberOfKeys. This is unlike purchase, which has the notSoldOut modifier. grantKey: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinGrantKeys.sol#L41:#L42 shareKey: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinTransfer.sol#L83:#L84 Both functions call _assignNewTokenId which does not check maxNumberOfKeys. https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinKeys.sol#L311:#L322 So you can say that _assignNewTokenId is actually the root of the error, and this is why I am submitting this as 1 finding and not 2 (for grantKey/shareKey).

Recommended Mitigation Steps

Add a check to _assignNewTokenId that will revert if we need to record a new key and maxNumberOfKeys has been reached.