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: 10/36
Findings: 3
Award: $3,528.88
🌟 Selected for report: 3
🚀 Solo Findings: 0
Jujic
A call to an arbitrary contract with custom calldata is made in arbitraryCall(address who, bytes memory data), which means the contract can be an ERC20 token, and the calldata can be transferFrom a previously approved user.
The wallet balances (for the amount up to the allowance limit) of the tokens that users approved to the Stream contract can be stolen.
The attacker create Stream contract on the Factory and malicious ERC20 contract
Bob has approved 1000 tokens on Stream contract
Attacker call function:
arbitraryCall(address who, bytes memory data) public lock externallyGoverned { where: who = address arbitrary malicious ERC20 contract data = abi.encodeWithSignature( "transferFrom(address,address,uint256)", address(Bob), any address hacker, 1000 tokens ) ... (bool success, bytes memory _ret) = who.call(data); require(success);
As a result, 1000 token will be stolen from Bob and sent to the attacker.
Remix
You can remove this dangerous function from the protocol.
#0 - 0xean
2022-01-14T21:40:17Z
dupe #258
🌟 Selected for report: Jujic
805.8152 USDC - $805.82
Jujic
In the fundStream() function, any user can avoid fee payments using small amount several times.
Assume that the fee - 0.1%
The fee amount will calculate:
require(amount > 0, "amt");// amount = 999 ... feeAmt = uint112(uint256(feePercent) * uint256(amount) / 10000); // feeAmt = 10 * 999 / 10000); // feeAmt = 0
I think it is not serious because these amounts are some little, but if the tokens have small decimals it will make a difference.
Add check for minimal amount
#0 - brockelmore
2021-12-06T16:10:54Z
duplicate #148
#1 - brockelmore
2021-12-06T16:54:10Z
Misread this report. Disagree with severity. Noncritical imo. We dont want to collect dust fees anyway.
#2 - 0xean
2022-01-15T01:50:06Z
Marking down to low-risk as the costs of doing so would probably outweigh any benefits on any chain with reasonable gas prices. Increasing the number of decimals used to calc fees could resolve this issue pretty easily.
🌟 Selected for report: hack3r-0m
Also found by: Jujic, toastedsteaksandwich
217.5701 USDC - $217.57
Jujic
In the flashloan() function, any user can avoid fee payments using small amount several times.
uint112 feeAmt = amount * 10 / 10000; // feeAmt = 999 * 10 / 10000 = 0
Visual Studio
Add check
require (feeAmt > 0, "Small amount");
#0 - 0xean
2022-01-15T01:52:25Z
dupe of #221
79.3043 USDC - $79.30
Jujic
Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.
The advantages of versions 0.8.* over <0.8.0 are:
Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.) Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls. Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs. Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.
Consider to upgrade pragma to at least 0.8.4.
#0 - brockelmore
2021-12-06T17:00:48Z
We define in .dapprc that the compiler version is 0.8.10, which is what is used to deploy
#1 - 0xean
2022-01-16T13:16:44Z
dupe of #19
Jujic
All the Stream contract methods could be set external instead public to save gas.
#0 - 0xean
2022-01-16T14:26:14Z
dupe of #260
🌟 Selected for report: ye0lde
Also found by: 0x0x0x, Jujic, pedroais, pmerkleplant
13.1496 USDC - $13.15
Jujic
!= 0 is a cheaper operation compared to > 0, when dealing with uint.
VSC
#0 - 0xean
2022-01-16T14:20:19Z
dupe of #143
🌟 Selected for report: GiveMeTestEther
27.0568 USDC - $27.06
Jujic
The state variable merkleAccess in struct TokenStream is initialized, but the contract never writes a value to it, nor is its value read anywhere.
Remove unused variable.
#0 - 0xean
2022-01-16T14:50:21Z
dupe of #42
🌟 Selected for report: Jujic
Jujic
ts.lastUpdate can be cached to slightly reduce gas usage
VSC
Consider caching those variable for read and make sure write back to storage
🌟 Selected for report: Jujic
100.2104 USDC - $100.21
Jujic
You can use in one require several checks to save gas.
require (isSale && !claimedDepositTokens && msg.sender == streamCreator && block.timestamp >= endStream, "claim error");
#0 - brockelmore
2021-12-06T16:14:33Z
Not a tradeoff i want to make in terms of code readability