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: 17/26
Findings: 3
Award: $241.81
🌟 Selected for report: 1
🚀 Solo Findings: 0
Reigada
In the contract MixinPurchase and also PublicLock as PublicLock inherits from MixinPurchase, the purchase function assumes that the smart contract will receive the 100% of the pricePaid amount as can be seen in:
https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinPurchase.sol#L97
https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinPurchase.sol#L103
However, this may not be true if the token is a transfer-on-fee token or a deflationary/rebasing token, causing the received amount by the smart contract to be less than the one set in pricePaid. This would still pass the require check require(pricePaid >= inMemoryKeyPrice, 'INSUFFICIENT_VALUE'); but actually the smart contract would receive less tokens than inMemoryKeyPrice.
https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinPurchase.sol#L97 https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinPurchase.sol#L103
Manual code review
Get the actual received amount by calculating the difference of token balance before and after the transfer. For example:
uint256 balanceBefore = token.balanceOf(address(this)); token.transferFrom(msg.sender, address(this), pricePaid); uint256 receivedAmount = token.balanceOf(address(this)) - balanceBefore; require(receivedAmount >= inMemoryKeyPrice, 'INSUFFICIENT_VALUE');
#0 - julien51
2021-11-23T03:31:18Z
That is a fair point. That said I would not classify this as "high risk". Basically for this to be an issue, someone would have to intentionally deploy a lock with such an ERC20 for them to be impacted. I would assume that when someone deploys a lock with a specific ERC20 that is because they decide to be paid with it, which means that they should know about the fact that it has these characteristics.
#1 - 0xleastwood
2022-01-16T05:50:45Z
I think this is a very unlikely issue. There are no direct or indirect risk of funds and the availability of the protocol is not impacted. However, it does seem that this might cause small issues to unaware content creators. Marking this as low severity.
#2 - 0xleastwood
2022-01-16T09:10:10Z
Duplicate of #162
#3 - 0xleastwood
2022-01-18T13:08:33Z
Bumped to medium as #221 mentions this as a potential risk
Reigada
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met.
Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinRoles.sol#L40 https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinRoles.sol#L45 https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L241 https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/Unlock.sol#L246 https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/UnlockDiscountTokenV2.sol#L27
Manual testing
Shorten the revert strings to fit in 32 bytes.
Or in contracts using solc version 0.8.4 or greater use the Custom Errors feature.
#0 - 0xleastwood
2022-01-17T08:28:41Z
I think this is likely useful.
🌟 Selected for report: GiveMeTestEther
14.4834 USDC - $14.48
Reigada
In all the loops, the variable i is incremented using i++. It is known that using ++i costs less gas per iteration than i++.
mixins/MixinGrantKeys.sol:29: for(uint i = 0; i < _recipients.length; i++) { UnlockUtils.sol:59: for (uint i = 0; i < 20; i++) {
Manual testing
Use ++i instead of i++ to increment the value of an uint variable.
#0 - 0xleastwood
2022-01-17T08:34:27Z
Duplicate of #149
Reigada
In multiple contracts, there are constant state variables declared the following way:
mixins/MixinRoles.sol:12: bytes32 public constant LOCK_MANAGER_ROLE = keccak256("LOCK_MANAGER"); mixins/MixinRoles.sol:13: bytes32 public constant KEY_GRANTER_ROLE = keccak256("KEY_GRANTER");
ERC20Patched.sol:824: bytes32 private constant _TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
ERC20Patched.sol:1385: bytes32 private constant _DELEGATION_TYPEHASH = ERC20Patched.sol-1386- keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)");
This results in the keccak256 operation being performed whenever the variable is used, increasing gas costs relative to just storing the output hash.
https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/mixins/MixinRoles.sol#L12-L13 https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/ERC20Patched.sol#L824 https://github.com/code-423n4/2021-11-unlock/blob/main/smart-contracts/contracts/ERC20Patched.sol#L1385-L1386
Manual testing
It is recommended to either:
#0 - 0xleastwood
2022-01-17T08:27:35Z
If I'm not mistaken, keccak hashes are calculated during compile time. So there aren't inherent savings and there is a huge trade-off on readability.
#1 - 0xleastwood
2022-01-17T09:14:23Z
I might be wrong here, marking as duplicate of #238
Reigada
In the contract MixinLockCore and also PublicLock (as PublicLock inherits from MixinLockCore) we can find the following function:
function approveBeneficiary( address _spender, uint _amount ) public onlyLockManagerOrBeneficiary returns (bool) { return IERC20Upgradeable(tokenAddress).approve(_spender, _amount); }
Some tokens (like USDT) don’t correctly implement the EIP20 standard and their approve/transfer/transferFrom functions return void, instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert as it is happening here.
Manual code review
It is recommended to use SafeERC20 - safeApprove.
#0 - julien51
2022-01-03T12:11:15Z
Thanks, that's a good one. I am not sure any funds are at risk tho so I would qualify this as a 1.
#1 - 0xleastwood
2022-01-16T09:01:27Z
Agree with sponsor here. Marking as low.
#2 - 0xleastwood
2022-01-16T09:09:10Z
Duplicate of #161