Platform: Code4rena
Start Date: 30/11/2021
Pot Size: $100,000 USDC
Total HM: 15
Participants: 36
Period: 7 days
Judge: 0xean
Total Solo HM: 4
Id: 62
League: ETH
Rank: 16/36
Findings: 3
Award: $2,269.54
🌟 Selected for report: 6
🚀 Solo Findings: 0
🌟 Selected for report: GiveMeTestEther
Also found by: ScopeLift
1087.8505 USDC - $1,087.85
GiveMeTestEther
"arbitraryCall()"'s (L733) use case is to claim airdrops by "gov". If the address "who" is a token that could be send as an incentive by an attacker via "createIncentive()" then such claim can be made unusable, because on L735 there is a require(incentives[who] == 0, "inc"); that reverts if a "who" token was received as an incentive.
In this case the the incentives[who] can be set to 0 by the stream creator by calling "claimIncentive()" but only after the stream has ended according to "require(block.timestamp >= endStream, "stream");" (L520)
If the airdrop is only claimable before the end of the stream, then the airdrop can never be claimed.
If "gov" is not the stream creator then the stream creator must become also the "gov" because "claimIncentive()" only can be called by the stream creator and the "arbitraryCall()" only by "gov". If resetting incentives[who] to 0 by calling "claimIncentive()" and "arbitraryCall()" for the "who" address doesn't happen atomic, an attacker can send between those two calls again a "who" token.
#0 - brockelmore
2021-12-01T22:15:20Z
Yep this is the tradeoff being made. To maintain trustlessness, we cannot remove the incentives[who] == 0 check. Additionally, governance shouldn't be in charge of an arbitrary stream's recoverTokens function.
The upshot of this is most MerkleDrop contracts are generally external of the token itself and not baked into the ERC20 itself. If a user wants to grief governance, they could continuously createIncentive after the stream creator claims the previous. But it does cost the user.
🌟 Selected for report: GiveMeTestEther
27.0568 USDC - $27.06
GiveMeTestEther
Remove the unused merkleAccess variable in the TokenStream struct. According to the struct packing it uses a single storage slot.
#0 - brockelmore
2021-12-01T21:31:37Z
Ye this is a remnant from a feature we removed.
#1 - brockelmore
2021-12-08T22:45:51Z
duplicates: #164 and #257
🌟 Selected for report: gpersoon
Also found by: GiveMeTestEther, Meta0xNull, bitbopper, hack3r-0m, pauliax, pedroais, wuwe1
48.1774 USDC - $48.18
GiveMeTestEther
The modifier "updateStreams" (L197) takes an argument "address who" and passes it to the function "updateStreamInternal" (L203). But this argument is never used in "updateStreamInternal" function. To save gas by passing arguments that are not used, remove function parameter "address who" from the modifier and the function declaration.
#0 - 0xean
2022-01-16T14:51:21Z
dupe of #125
🌟 Selected for report: GiveMeTestEther
100.2104 USDC - $100.21
GiveMeTestEther
Save gas by caching the return value from rewardPerToken() in a local variable and use the local variable L220 and L222. This saves us two storage reads (1 cold = 800 gas, and 1 warm= 100 gas). It is way cheaper to read from a local variable (push/pop operations 2-3 gas each + cheap others)
Note: same for the claimReward() function on L555
🌟 Selected for report: GiveMeTestEther
GiveMeTestEther
On L294/295/296 storage variable are assigned with the values of an expression consisting of storage variables, but we could reuse the function arguments that were used to set the storage variable of the expression. This saves us 6 SLOAD.
Manual Analysis
#0 - brockelmore
2021-12-01T21:49:53Z
There are no SLOADs performed because all of the referenced variables are immutables themselves, see here: https://docs.soliditylang.org/en/v0.8.10/contracts.html#constant-and-immutable-state-variables
That being said, it may be slightly more gas efficient, i would have to test
🌟 Selected for report: GiveMeTestEther
GiveMeTestEther
On the L388/L429/L464 (only subtraction)/L508 there is a subtraction than can be done unchecked because the require statement above those line checks for underflow.
Therefore those lines can be put in an unchecked {} block to save gas.
Example for L388: require(newBal < type(uint112).max && newBal > prevBal, "erc"); amount = uint112(newBal - prevBal);
First part of the require statement checks for safe downcasting and the second part for underflow. Therefore we can write: unchecked { amount = uint112(newBal - prevBal); }
🌟 Selected for report: GiveMeTestEther
805.8152 USDC - $805.82
GiveMeTestEther
Only ERC20 tokens with a decimals() function can be used as a depositToken. A stream creator maybe not be aware of this restriction and the creation of a stream would revert.
In the constructor of the Stream contract the decimals() (L310) functions of the depositToken is called. But according to EIP20 (https://eips.ethereum.org/EIPS/eip-20) the decimals() function is optional.