Unlock Protocol contest - GiveMeTestEther's results

Handle

GiveMeTestEther

Vulnerability details

Impact / POC

A user can share the minimum possible time of his key (and buy additional time by extending the key to have enough time to make the attack work) to a recipient of the "grantKeys()" such that for this recipient the "require(expirationTimestamp > toKey.expirationTimestamp, 'ALREADY_OWNS_KEY');" reverts. This will revert the whole transaction and no recipient will receive an airdropped key.

An attacker can optimize for which recipient & key expiration timestamp to maximize gas griefing.

This attack is also feasible, by buying a key for a recipient or transferring his "key" to the recipient (buys for himself and transfers to the recipient) if this allows the "require(expirationTimestamp > toKey.expirationTimestamp, 'ALREADY_OWNS_KEY');" to revert.

grantKeys(): https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinGrantKeys.sol#L22

Tools Used

Manual Analysis

Recommended Mitigation Steps

• skip the users that satisfy the require condition (change to an if statement) and don't emit an event for skipped users
• or create a new event for key granting that differentiates by user that received an airdrop an by users that didn't

Handle

GiveMeTestEther

Vulnerability details

Impact

• use safeApprove from SafeERC20 instead of approve, because token can be arbitrary
• call safeApprove(_spender, 0) before calling safeApprove(_spender, _amount)

Proof of Concept

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

• reason for setting safeApprove(0): https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/edit

Tools Used

Manual Analysis

Recommended Mitigation Steps

token.safeApprove(_spender, 0); token.safeApprove(_spender, _amount);

Handle

GiveMeTestEther

Vulnerability details

Impact / POC

A single user can become the owner of multiple token ids and break the assumption of the comment https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinKeys.sol#L181 of the function numberOfOwners() that it returns "total number of unique owners"

If a key manager/approved transfers a key with transferFrom() https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinTransfer.sol#L109 to a recipient that also owns a valid key then we don't go into the "if block" L131 and also not into the "if block" L138 (this is important s.t. no key owner change happens) and go into the "else block" L148 (not really important).

We end with: fromKey.expirationTimestamp = block.timestamp; and fromKey.tokenId = 0;

If the key owner or someone else buys for this key owner again a "key" https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinPurchase.sol#L51 satisfies the condition idTo ==0 (bcs of tokenId = 0) and in _assignNewTokenId(toKey); the key gets a new token id and the owner gets also registered as the new owner of the new token id in _recordOwner(_recipient, idTo);

The "old" key got overwritten but we are now the owner of two token ids.

This breaks the comment of numberOfOwners() that it returns "total number of unique owners" but for this the key owner that owns now two token ids, we executed "_recordOwner" twice and therefore added the same address twice to the owner array https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinKeys.sol#L327

Tools Used

Manual Analysis

Recommended Mitigation Steps

• need to also implement the removal of ownership of a tokenId when it is set 0 zero to be congruent with the state of the key, and also adapt the other logic depending on it

Handle

GiveMeTestEther

Vulnerability details

Impact

If a user has timeRemaining = key.expirationTimestamp - block.timestamp; (L142) bigger than "expirationDuration" (lock config storage variable) by (e.g. purchasing/extending key time) the assumption from the user would be to be able to refund more than "keyPrice". But the maximal refund amount in "_getCancelAndRefundValue()" is bounded by "keyPrice" (L143)

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

A user can get to this state by extending/purchasing key or receiving time from sharing/transferring key time from another user.

Proof of Concept

extreme case:

• the user purchases twice a key in the same block (the second purchase extends the key additionally by expirationDuration)
• this results in key.expirationTimestamp = block.timestamp + 2 * expirationDuration
• if the user calls "cancelAndRefund()" (also same block) the if statement is true even if "freeTrialLength" is 0:
  • block.timestamp + 2 * expirationDuration - block.timestamp + freeTrialLength >= expirationDuration
  • ⇒ 2 * expirationDuration >= expirationDuration
• on line L144 we set "refund = keyPrice" but from the users view, he should get back 2 * "keyPrice"

Tools Used

Manual Analysis

Recommended Mitigation Steps

• Clearly state that an early key extension may result in a disadvantage for refunding and that always maximal "keyPrice" can be refunded.
• change calculation and accounting, but this can get complicated

Handle

GiveMeTestEther

Vulnerability details

Impact

Save gas by writing unchecked {timeToTransfer = key.expirationTimestamp -**** block.timestamp;} bcs the getHasValidKey(_keyOwner) checks if keyByOwner[_keyOwner].expirationTimestamp > block.timestamp; and therefore we can save gas by not doing the under/overflowflow check.

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

Tools Used

Manual Analysis

Recommended Mitigation Steps

• {timeToTransfer = key.expirationTimestamp -**** block.timestamp;}

Handle

GiveMeTestEther

Vulnerability details

Impact

In the function _getCancelAndRefundValue we can save gas by using the unchecked {} block in L142 and L154.

Proof of Concept

• L142: the modifier hasValidKey checks key.expirationTimestamp > block.timestamp and therefore no underflow can happen in this subtraction: we can write unchecked {uint timeRemaining = key.expirationTimestamp - block.timestamp;}
• L154: the if statement check "refund > penalty" and therefore no underflow can happen in this subtraction: we can write unchecked {refund -= penalty;}

Tools Used

Manual Analysis

Recommended Mitigation Steps

• L142: unchecked {uint timeRemaining = key.expirationTimestamp - block.timestamp;}
• L154: unchecked {refund -= penalty;}

Handle

GiveMeTestEther

Vulnerability details

Impact

• (1.1) save gas by incrementing the loop index (uint256) in an unchecked {}, because we would run out of gas before if the loop index overflows uint256
• (1.2) write ++i (more gas efficient) instead of i++
• store the array length in a local variable to save gas by not reading the array length from calldata in each loop iteration

Proof of Concept

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

Tools Used

• Manual Analysis

Recommended Mitigation Steps

• (1.1 & 1.2): write the loop index increment i++ as unchecked { ++ i }
• (1.3) store length in a local var: uint recicipientsLength = _recipients.length; and then write the index bound check as i < recicipientsLength

Handle

GiveMeTestEther

Vulnerability details

Impact

don't need to set the local variable newLock as payable on L220 of the function createLock, we save gas by not casting the proxy address

Proof of Concept

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/Unlock.sol#L220

Tools Used

Manual Analysis

Recommended Mitigation Steps

• replace L220 with: address newLock = address(proxy);

Handle

GiveMeTestEther

Vulnerability details

Impact

the return value of the private _deployProxyAdmin() function is not used in initialize() and initializeProxyAdmin(), therefore the _deployProxyAdmin() function doesn't need to return a value and we can save gas

Proof of Concept

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/Unlock.sol#L162

Tools Used

Manual Analysis

Recommended Mitigation Steps

• change _deployProxyAdmin() to no return a value

Handle

GiveMeTestEther

Vulnerability details

Recommended Mitigation Steps

Move the require statement to the beginning of the function so save gas in the case of a revert (no storage write) https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinFunds.sol#L24

Tools Used

Manual Analysis