Unlock Protocol contest - cmichel's results

Handle

cmichel

Vulnerability details

The locks implement three different approval types, see onlyKeyManagerOrApproved for an overview:

• key manager (map keyManagerOf)
• single-person approvals (map approved). Cleared by _clearApproval or _setKeyManagerOf
• operator approvals (map managerToOperatorApproved)

The MixinTransfer.transferFrom requires any of the three approval types in the onlyKeyManagerOrApproved modifier on the tokenId to authenticate transfers from from.

Notice that if the to address previously had a key but it expired only the _setKeyManagerOf call is performed, which does not clear approved if the key manager was already set to 0:

function transferFrom(
  address _from,
  address _recipient,
  uint _tokenId
)
  public
  onlyIfAlive
  hasValidKey(_from)
  onlyKeyManagerOrApproved(_tokenId)
{
  // @audit this is skipped if user had a key that expired
  if (toKey.tokenId == 0) {
    toKey.tokenId = _tokenId;
    _recordOwner(_recipient, _tokenId);
    // Clear any previous approvals
    _clearApproval(_tokenId);
  }

  if (previousExpiration <= block.timestamp) {
    // The recipient did not have a key, or had a key but it expired. The new expiration is the sender's key expiration
    // An expired key is no longer a valid key, so the new tokenID is the sender's tokenID
    toKey.expirationTimestamp = fromKey.expirationTimestamp;
    toKey.tokenId = _tokenId;

    // Reset the key Manager to the key owner
    // @audit  doesn't clear approval if key manager already was 0
    _setKeyManagerOf(_tokenId, address(0));

    _recordOwner(_recipient, _tokenId);
  }
  // ...
}

// 
function _setKeyManagerOf(
  uint _tokenId,
  address _keyManager
) internal
{
  // @audit-ok only clears approved if key manager updated
  if(keyManagerOf[_tokenId] != _keyManager) {
    keyManagerOf[_tokenId] = _keyManager;
    _clearApproval(_tokenId);
    emit KeyManagerChanged(_tokenId, address(0));
  }
}

Impact

It's possible to sell someone a key and then claim it back as the approvals are not always cleared.

POC

• Attacker A has a valuable key (tokenId = 42) with an expiry date far in the future.
• A sets approvals for their second attacker controlled account A' by calling MixinKeys.setApprovalForAll(A', true), which sets managerToOperatorApproved[A][A'] = true.
• A clears the key manager by setting it to zero, for example, by transferring it to a second account that does not have a key yet, this calls the above _setKeyManagerOf(42, address(0)); in transferFrom
• A sets single-token approval to A' by calling MixinKeys.approve(A', 42), setting approved[42] = A'.
• A sells the token to a victim V for a discount (compared to purchasing it from the Lock). The victim needs to have owned a key before which already expired. The transferFrom(A, V, 42) call sets the owner of token 42 to V, but does not clear the approved[42] == A' field as described above. (_setKeyManagerOf(_tokenId, address(0)); is called but the key manager was already zero, which then does not clear approvals.)
• A' can claim back the token by calling transferFrom(V, A', 42) and the onlyKeyManagerOrApproved(42) modifier will pass as approved[42] == A' is still set.

Recommended Mitigation Steps

The _setKeyManagerOf function should not handle clearing approvals of single-token approvals (approved) as these are two separate approval types. The transferFrom function should always call _clearApproval in the (previousExpiration <= block.timestamp) case.

Handle

cmichel

Vulnerability details

Some tokens (like USDT) don't correctly implement the EIP20 standard and their transferFrom function returns void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert.

For the tokens that return a success value, the contract does not check it.

Non-safe transfers are used in:

• MixinPurchase.purchase: token.transferFrom(msg.sender, address(this), pricePaid);

Impact

Tokens that return false on a failed transferFrom or that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the protocol as they revert the transaction because of the missing return value.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransferFrom function that handle the return value check as well as non-standard-compliant tokens.

Handle

cmichel

Vulnerability details

The MixinTransfer.shareKey function does not disallow transfering the key to oneself, i.e. from == recipient.

When doing a self-transfer, the remaining time for oneself should be reduced by the fees, but the keys immediately expire due to explicitly setting the expiry on fromKey:

// @audit self-transfer from == recipient expires this key and sets tokenid to 0
fromKey.expirationTimestamp = block.timestamp;

Recommended Mitigation Steps

An easy fix is to disallow self-transfers as they have no use. Add a require(_from != _recipient, "!self") statement.

Handle

cmichel

Vulnerability details

The Unlock.recordKeyPurchase function is called on each key purchase (MixinPurchase.purchase) and mints UDT tokens to the referrer. The amount to mint is based on the transaction's gas price which is controlled by the caller (purchaser):

uint tokensToDistribute = (estimatedGasForPurchase * tx.gasprice) * (125 * 10 ** 18) / 100 / udtPrice;

Impact

Tokens can be minted by purchasing a key with themself as the referrer at a high transaction gas price. Depending on the UDT price on external markets, it could be profitable to buy a key at a high gas price, receive UDT and then sell them on a market for a profit.

Recommended Mitigation Steps

The amount minted should be more predictable and not depend on the user's gas price input. Consider declaring an average gas price storage variable that is set by a trusted party and use this one instead.

Handle

cmichel

Vulnerability details

The MixinTransfer.shareKey function wants to compute a fee such that time + fee * time == timeRemaining (timePlusFee):

uint fee = getTransferFee(keyOwner, _timeShared);
uint timePlusFee = _timeShared + fee;

However, if the time remaining is less than the computed fee time, the computation changes and a different formula is applied. The fee is now simply taken on the remaining time.

if(timePlusFee < timeRemaining) {
  // now we can safely set the time
  time = _timeShared;
  // deduct time from parent key, including transfer fee
  _timeMachine(_tokenId, timePlusFee, false);
} else {
  // we have to recalculate the fee here
  fee = getTransferFee(keyOwner, timeRemaining);
  // @audit want it such that time + fee * time == timeRemaining, but fee is taken on timeRemaining instead of time
  time = timeRemaining - fee;
}

It should compute the time without fee as time = timeRemaining / (1.0 + fee_as_decimal) instead, i.e., time = BASIS_POINTS_DEN * timeRemaining / (transferFeeBasisPoints + BASIS_POINTS_DEN).

POC

To demonstrate the difference with a 10% fee and a _timeShared = 10,000s which should be credited to the to account.

The correct time plus fee which is reduced from from (as in the timePlusFee < timeRemaining branch) would be 10,000 + 10% * 10,000 = 11,000.

However, if from has not enough time remaining and timePlusFee >= timeRemaining, the entire time remaining is reduced from from but the credited time is computed wrongly as: (Let's assume timeRemaining == timePlusFee): time = 11,000 - 10% * 11,000 = 11,000 - 1,100 = 9900.

They would receive 100 seconds less than what they are owed.

Impact

When transferring more time than the from account has, the credited time is scaled down wrongly and the receiver receives less time (a larger fee is applied).

Recommended Mitigation Steps

It should change the first if branch condition to timePlusFee <= timeRemaining (less than or equal). In the else branch, it should compute the time without fee as time = BASIS_POINTS_DEN * timeRemaining / (transferFeeBasisPoints + BASIS_POINTS_DEN).

Handle

cmichel

Vulnerability details

The Unlock.recordKeyPurchase function computes the maxTokens as:

maxTokens = IMintableERC20(udt).balanceOf(address(this)) * valueInETH / (2 + 2 * valueInETH / grossNetworkProduct) / grossNetworkProduct;

Note that grossNetworkProduct was already increased by valueInETH in the code before. Meaning, the (2 + 2 * valueInETH / grossNetworkProduct) part of the computation will almost always be 2 as usually grossNetworkProduct > 2 * valueInETH, and thus the 2 * valueInETH / grossNetworkProduct is zero by integer division.

Impact

The maxTokens curve might not be computed as intended and lead to being able to receive more token rewards than intended.

Recommended Mitigation Steps

The comment "we distribute tokens using asymptotic curve between 0 and 0.5" should be more clear to indicate how exactly the curve looks like. It could be that a floating-point number was desired instead of the integer division in 2 * valueInETH / grossNetworkProduct. In that case, consider adding a scaling factor to this term and divide by it at the end of the computation again.