Platform: Code4rena
Start Date: 18/11/2021
Pot Size: $50,000 USDC
Total HM: 18
Participants: 26
Period: 7 days
Judge: leastwood
Total Solo HM: 12
Id: 54
League: ETH
Rank: 10/26
Findings: 2
Award: $868.39
🌟 Selected for report: 2
🚀 Solo Findings: 0
Ruhum
Not every ERC20 token reverts if an operation was unsuccessful. Instead they might just return false.
If the return value is not properly checked, the contract might ignore the fact that the transfer failed.
https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#SafeERC20
manual analysis
Use the SafeERC20 functions which revert in case the underlying token returns false instead of reverting.
#0 - julien51
2022-01-03T11:56:44Z
Like all other issues related to the use of arbitrary ERC20, I think this is quite limited. We actually want to remain "liberal" here by accepting any ERC20, because it has no impact on the protocol's behavior directly, however this does expect that lock creators will have a good understanding of the challenges of using a custom ERC20 contract.
#1 - 0xleastwood
2022-01-16T05:30:20Z
Duplicate of #162
🌟 Selected for report: Ruhum
398.4587 USDC - $398.46
Ruhum
When working with ERC20 tokens, the approve function is susceptible to a race condition. The beneficiary can spend their current approved amount right before a new amount is approved. Thus allowing them to spend more than intended by the owner of the funds.
A detailed explanation can be found here: https://swcregistry.io/docs/SWC-114#description
Manual Analysis
Use safeIncreaseAllowance and safeDecreaseAllowance instead. https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#SafeERC20-safeIncreaseAllowance-contract-IERC20-address-uint256-
The SafeERC20 methods are recommended here because the token can be any arbitrary once. Not all tokens revert if an operation failed. Thus it's best to use safe methods.
#0 - 0xleastwood
2022-01-16T09:17:11Z
I think this should actually be of low severity.
🌟 Selected for report: Ruhum
398.4587 USDC - $398.46
Ruhum
If the Uniswap oracle can't be found for the token of the referred lock, the variable valueInETH might never be initialized. In that case, the function would work with the variables zero value which is 0 without throwing any error in any way. That might lead to unexpected behavior. The scenario seems to be only possible if the token's oracle address was set to a 0 address by the owner (which is technically possible).
The issue seems to also only affect the recording and not the actual purchase as far as I am aware. Better to double-check this tho!
Relevant part in recordKeyPurchase(): https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L297-L305
If the oracle is a zero address, valueInETH is never assigned a value: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L302
The function where the oracle address is set by the owner: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L451-L455 It's technically possible to set it to a zero address since there are no address checks.
Only affects the recording here: https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinPurchase.sol#L89 The actual purchasing logic seems to be fine.
Slither reported a possible scenario where the variable is not initialized.
Not sure whether reverting, in this case, might make sense.