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: 3/36
Findings: 7
Award: $7,936.26
🌟 Selected for report: 16
🚀 Solo Findings: 0
385.4189 USDC - $385.42
WatchPug
uint256 excess = ERC20(token).balanceOf(address(this)) - (depositTokenAmount - redeemedDepositTokens);
In the current implementation, depositTokenFlashloanFeeAmount is not excluded when calculating excess depositToken. Therefore, the stream creator can call recoverTokens(depositToken, recipient) and retrieve depositTokenFlashloanFeeAmount if there are any.
As a result:
governance calls claimFees() and claim accumulated depositTokenFlashloanFeeAmount, it may fail due to insufficient balance of depositToken.governance as fees, causing some users unable to withdraw or can only withdraw part of their deposits.Given:
feeEnabled: truefeePercent: 10 (0.1%)1,000,000 depositToken;flashloan() and borrowed 1,000,000 depositToken, then repaid 1,001,000;1,000 depositToken;endDepositLock, Alice called claimDepositTokens() and withdrawn 1,000,000 depositToken;streamCreator called recoverTokens(depositToken, recipient) and retrieved 1,000 depositToken (2,000 - (1,001,000 - 1,000,000));governance called claimFees() and retrieved another 1,000 depositToken;claimDepositTokens() but since the current balanceOf depositToken is 0, the transcation always fails, and Charlie loses all the depositToken.Change to:
uint256 excess = ERC20(token).balanceOf(address(this)) - (depositTokenAmount - redeemedDepositTokens) - depositTokenFlashloanFeeAmount;
#0 - brockelmore
2021-12-08T22:16:11Z
duplicates: #211, #206, #195, #182, #149, #129, #128, #120, #86
2175.7011 USDC - $2,175.70
WatchPug
function arbitraryCall(address who, bytes memory data) public lock externallyGoverned { // cannot have an active incentive for the callee require(incentives[who] == 0, "inc"); ...
When an incentiveToken is claimed after endStream, incentives[who] will be 0 for that incentiveToken.
If the protocol gov is malicious or compromised, they can call arbitraryCall() with the address of the incentiveToken as who and transferFrom() as calldata and steal all the incentiveToken in the victim's wallet balance up to the allowance amount.
USDC to the streaming contract;createIncentive() and added 1,000 USDC of incentive;claimIncentive() and claimed 1,000 USDC;The compromised protocol gov can call arbitraryCall() and steal all the USDC in Alice's wallet balance.
Consider adding a mapping: isIncentiveToken, setting isIncentiveToken[incentiveToken] = true in createIncentive(), and require(!isIncentiveToken[who], ...) in arbitraryCall().
#0 - brockelmore
2021-12-08T22:32:56Z
duplicates: #242, #229
WatchPug
In the current implementation, ts.lastUpdate will only be updated when ts.tokens > 0. Thus, ts.lastUpdate can be outdated for an exited user who deposits again.
As a result, by the next time updateStreamInternal() is called, ts.tokens will be reduced more than expected mistakenly.
modifier updateStream(address who) { // save bytecode space by making it a jump instead of inlining at cost of gas updateStreamInternal(who); _; } function updateStreamInternal(address who) internal { ... // update users unstreamed balance uint32 acctTimeDelta = uint32(block.timestamp) - ts.lastUpdate; if (acctTimeDelta > 0 && ts.tokens > 0) { // some time has passed since this user last interacted // update ts not yet streamed ts.tokens -= uint112(acctTimeDelta * ts.tokens / (endStream - ts.lastUpdate)); ts.lastUpdate = uint32(block.timestamp); } ...
Given:
startTime: 1/1/2022endStream: 1/1/2023stake() and exit() right after, ts.lastUpdate is 1/1/2022;stake(1000e18). At L226, as ts.tokens == 0, ts.lastUpdate remains 1/1/2022;stake(1 wei):
ts.tokens remains 1000e18, and almost all of the 1000e18 can be withdrawn;ts.lastUpdate is not updated at step 2. At L225, acctTimeDelta will be ~11 mos. Thus, at L229 ts.tokens will be reduced to 83.33e18, and only 83.33e18 can be withdrawn.Change to:
// update users unstreamed balance uint32 acctTimeDelta = uint32(block.timestamp) - ts.lastUpdate; if (acctTimeDelta > 0 && ts.tokens > 0) { // some time has passed since this user last interacted // update ts not yet streamed ts.tokens -= uint112(acctTimeDelta * ts.tokens / (endStream - ts.lastUpdate)); } ts.lastUpdate = uint32(block.timestamp);
#0 - brockelmore
2022-01-05T17:07:35Z
duplicate #123
🌟 Selected for report: egjlmn1
Also found by: WatchPug, itsmeSTYJ, toastedsteaksandwich
WatchPug
function approve(address spender, uint256 amount) public virtual returns (bool) { allowance[msg.sender][spender] = amount; emit Approval(msg.sender, spender, amount); return true; }
Using approve() to manage allowances opens yourself and users of the token up to frontrunning. Best practice, but doesn't usually matter.
Explanation of this possible attack vector
See also: 0xProject/0x-monorepo#850
A potential fix includes preventing a call to approve if all the previous tokens are not spent through adding a check that the allowed balance is 0:
require(allowed[msg.sender][_spender] == 0).
#0 - 0xean
2022-01-16T14:06:17Z
duplicate of #74
🌟 Selected for report: harleythedog
Also found by: WatchPug, csanuragjain, gpersoon, hubble
WatchPug
unstreamed is a public variable, and it's been actively managed in stake(), updateStreamInternal(). However, since users can also withdraw unstreamed depositToken, the global variable unstreamed should be updated in withdraw() as well.
For example:
10,000 depositToken;10,000 depositToken right after step 1.unstreamed to be 0;unstreamed to be 10,000.function withdraw(uint112 amount) public lock updateStream(msg.sender) { require(amount > 0, "amt"); // checked in updateStream // is the stream still going on? thats the only time a depositer can withdraw // require(block.timestamp < endStream, "withdraw:!stream"); TokenStream storage ts = tokensNotYetStreamed[msg.sender]; require(ts.tokens >= amount, "amt"); ts.tokens -= amount; uint256 virtualBal = dilutedBalance(amount); ts.virtualBalance -= virtualBal; totalVirtualBalance -= virtualBal; depositTokenAmount -= amount; if (!isSale) { _burn(msg.sender, amount); } else { } // do the transfer ERC20(depositToken).safeTransfer(msg.sender, amount); emit Withdrawn(msg.sender, amount); }
Change to:
function withdraw(uint112 amount) public lock updateStream(msg.sender) { require(amount > 0, "amt"); // checked in updateStream // is the stream still going on? thats the only time a depositer can withdraw // require(block.timestamp < endStream, "withdraw:!stream"); TokenStream storage ts = tokensNotYetStreamed[msg.sender]; require(ts.tokens >= amount, "amt"); ts.tokens -= amount; unstreamed -= amount; uint256 virtualBal = dilutedBalance(amount); ts.virtualBalance -= virtualBal; totalVirtualBalance -= virtualBal; depositTokenAmount -= amount; if (!isSale) { _burn(msg.sender, amount); } else { } // do the transfer ERC20(depositToken).safeTransfer(msg.sender, amount); emit Withdrawn(msg.sender, amount); }
#0 - 0xean
2022-01-16T00:27:23Z
dupe of #118
WatchPug
Here are some examples that the code style does not follow the best practices:
function stake(uint112 amount) public lock updateStream(msg.sender) {
function recoverTokens(address token, address recipient) public lock {
function exit() public updateStream(msg.sender) {
#0 - 0xean
2022-01-16T14:09:30Z
Also a gas optimization, marking as such
18.2633 USDC - $18.26
WatchPug
updateStream() includes complex code execution, avoiding unnecessary internal call of updateStream() can save gas.
function exit() public updateStream(msg.sender) { // checked in updateStream // is the stream still going on? thats the only time a depositer can withdraw // require(block.timestamp < endStream, "withdraw:!stream"); TokenStream storage ts = tokensNotYetStreamed[msg.sender]; uint112 amount = ts.tokens; withdraw(amount); }
function withdraw(uint112 amount) public lock updateStream(msg.sender) { require(amount > 0, "amt"); // checked in updateStream // is the stream still going on? thats the only time a depositer can withdraw // require(block.timestamp < endStream, "withdraw:!stream"); TokenStream storage ts = tokensNotYetStreamed[msg.sender]; require(ts.tokens >= amount, "amt"); ts.tokens -= amount; uint256 virtualBal = dilutedBalance(amount); ts.virtualBalance -= virtualBal; totalVirtualBalance -= virtualBal; depositTokenAmount -= amount; if (!isSale) { _burn(msg.sender, amount); } else { } // do the transfer ERC20(depositToken).safeTransfer(msg.sender, amount); emit Withdrawn(msg.sender, amount); }
updateStream(msg.sender) at L487 is duplicated with updateStream(msg.sender) in withdraw() at L493.
Change to:
function withdraw(uint112 amount) public lock updateStream(msg.sender) { _withdraw(amount); } function _withdraw(uint112 amount) private { require(amount > 0, "amt"); // checked in updateStream // is the stream still going on? thats the only time a depositer can withdraw // require(block.timestamp < endStream, "withdraw:!stream"); TokenStream storage ts = tokensNotYetStreamed[msg.sender]; require(ts.tokens >= amount, "amt"); ts.tokens -= amount; uint256 virtualBal = dilutedBalance(amount); ts.virtualBalance -= virtualBal; totalVirtualBalance -= virtualBal; depositTokenAmount -= amount; if (!isSale) { _burn(msg.sender, amount); } else { } // do the transfer ERC20(depositToken).safeTransfer(msg.sender, amount); emit Withdrawn(msg.sender, amount); } /** * @dev Allows a stream depositor to exit their entire remaining tokens that haven't streamed * and burns receiptTokens if its not a sale. * * additionally, updates tokensNotYetStreamed. Lock is done in withdraw */ function exit() public lock updateStream(msg.sender) { // checked in updateStream // is the stream still going on? thats the only time a depositer can withdraw // require(block.timestamp < endStream, "withdraw:!stream"); TokenStream storage ts = tokensNotYetStreamed[msg.sender]; uint112 amount = ts.tokens; _withdraw(amount); }
#0 - 0xean
2022-01-17T12:26:27Z
dupe of #187
WatchPug
uint112 feeAmt = amount * 10 / 10000; // 10bps fee
Change to:
uint112 feeAmt = amount / 1000; // 10bps fee
#0 - 0xean
2022-01-17T12:50:55Z
dupe of #188
WatchPug
string public name; string public symbol;
name and symbol will never change, use immutable variable instead of storage variable can save gas.
Other examples include:
IGoverned public gov;
🌟 Selected for report: WatchPug
WatchPug
For the storage variables that will be accessed multiple times, cache and read from the stack can save ~100 gas from each extra read (SLOAD after Berlin).
For example:
/// Accepts governorship function acceptGov() public { require(pendingGov == msg.sender, "!pending"); address old = gov; gov = pendingGov; emit NewGov(old, pendingGov); }
pendingGov can be cached.
Change to:
/// Accepts governorship function acceptGov() public { address _pendingGov = pendingGov; require(_pendingGov == msg.sender, "!pending"); address old = gov; gov = _pendingGov; emit NewGov(old, _pendingGov); }
🌟 Selected for report: WatchPug
WatchPug
/// Update pending governor function setPendingGov(address newPendingGov) governed public { address old = pendingGov; pendingGov = newPendingGov; emit NewPendingGov(old, newPendingGov); }
old is unnecessary as it's being used only once. Can be changed to:
/// Update pending governor function setPendingGov(address newPendingGov) governed public { emit NewPendingGov(pendingGov, newPendingGov); pendingGov = newPendingGov; }
uint256 globalStreamingSpeedPerSecond = (uint256(unstreamed) * 10**6)/ (endStream - lastUpdate); unstreamed -= uint112((uint256(tdelta) * globalStreamingSpeedPerSecond) / 10**6);
Can be changed to:
unstreamed -= uint112((uint256(tdelta) * (uint256(unstreamed) * 10**6)/ (endStream - lastUpdate)) / 10**6);
function stake(uint112 amount) public lock updateStream(msg.sender) { require(amount > 0, "amt"); // checked in updateStream // require(block.timestamp < endStream, "stake:!stream"); // transfer tokens over uint256 prevBal = ERC20(depositToken).balanceOf(address(this)); ERC20(depositToken).safeTransferFrom(msg.sender, address(this), amount); uint256 newBal = ERC20(depositToken).balanceOf(address(this)); require(newBal <= type(uint112).max && newBal > prevBal, "erc"); uint112 trueDepositAmt = uint112(newBal - prevBal); depositTokenAmount += trueDepositAmt; TokenStream storage ts = tokensNotYetStreamed[msg.sender]; ts.tokens += trueDepositAmt; uint256 virtualBal = dilutedBalance(trueDepositAmt); ts.virtualBalance += virtualBal; totalVirtualBalance += virtualBal; unstreamed += trueDepositAmt; if (!isSale) { // not a straight sale, so give the user some receipt tokens _mint(msg.sender, trueDepositAmt); } else { } emit Staked(msg.sender, trueDepositAmt); }
trueDepositAmt can be replaced with amount.
function createIncentive(address token, uint112 amount) public lock { require(token != rewardToken && token != depositToken, "inc"); uint256 prevBal = ERC20(token).balanceOf(address(this)); ERC20(token).safeTransferFrom(msg.sender, address(this), amount); uint256 newBal = ERC20(token).balanceOf(address(this)); require(newBal <= type(uint112).max && newBal > prevBal, "erc"); uint112 amt = uint112(newBal - prevBal); incentives[token] += amt; emit StreamIncentivized(token, amt); }
amt can be replaced with amount.
(bool success, bytes memory _ret) = who.call(data);
Can be changed to:
(bool success, ) = who.call(data);
🌟 Selected for report: WatchPug
WatchPug
Per the Solidity docs:
When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
It might be beneficial to use reduced-size types if you are dealing with storage values because the compiler will pack multiple elements into one storage slot, and thus, combine multiple reads or writes into a single operation. If you are not reading or writing all the values in a slot at the same time, this can have the opposite effect, though: When one value is written to a multi-value storage slot, the storage slot has to be read first and then combined with the new value such that other data in the same slot is not destroyed.
Every contract has structs that pack multiple fields into slots by using < 256b types. This saves slots but increases runtime gas consumption due to masking of shared slot variables while reading/writing individual variables. The impact is more significant where the shared slot variables are not typically read/written together in functions which may allow the optimizer to combine their reads/writes in SLOADs/SSTOREs because that will not reduce SLOADs/SSTOREs used and instead add more bytecode/gas overhead for masking.
For example:
// == slot b == uint112 private rewardTokenFeeAmount; uint112 private depositTokenFlashloanFeeAmount; uint8 private unlocked = 1; bool private claimedDepositTokens; // ============
function lockInternal() internal { require(unlocked == 1, "re"); unlocked = 2; } modifier lock { lockInternal(); _; unlocked = 1; }
unlocked, rewardTokenFeeAmount, depositTokenFlashloanFeeAmount and claimedDepositTokens are packing in the same slot, but they are not typically read/written together.
WatchPug
For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.
For example:
require(ts.tokens >= amount, "amt"); ts.tokens -= amount;
ts.tokens -= amount will never overflow.
WatchPug
function toString(uint _i) internal pure returns (string memory) { if (_i == 0) { return "0"; } uint j = _i; uint len; while (j != 0) { len++; j /= 10; } bytes memory bstr = new bytes(len); uint k = len; while (_i != 0) { k = k-1; uint8 temp = (48 + uint8(_i - _i / 10 * 10)); bytes1 b1 = bytes1(temp); bstr[k] = b1; _i /= 10; } return string(bstr); }
Change to:
function toString(uint _i) internal pure returns (string memory) { if (_i == 0) { return "0"; } uint j = _i; uint len; while (j != 0) { len++; j /= 10; } bytes memory bstr = new bytes(len); uint k = len; while (_i != 0) { bstr[--k] = bytes1(uint8(48 + _i % 10)); _i /= 10; } return string(bstr); }
🌟 Selected for report: WatchPug
WatchPug
Redundant code increase contract size and gas usage at deployment.
if (!isSale) { // not a straight sale, so give the user some receipt tokens _mint(msg.sender, trueDepositAmt); } else { }
L443-444, else {} is redundant.
if (!isSale) { _burn(msg.sender, amount); } else { }
L472-473, else {} is redundant.
🌟 Selected for report: WatchPug
WatchPug
function dilutedBalance(uint112 amount) internal view returns (uint256) { // duration / timeRemaining * amount if (block.timestamp < startTime) { return amount; } else { uint32 timeRemaining = endStream - uint32(block.timestamp); return ((uint256(streamDuration) * amount * 10**6) / timeRemaining) / 10**6; } }
Change to:
function dilutedBalance(uint112 amount) internal view returns (uint256) { // duration / timeRemaining * amount if (block.timestamp < startTime) { return amount; } else { uint32 timeRemaining = endStream - uint32(block.timestamp); return ((uint256(streamDuration) * amount * 1e6) / timeRemaining) / 1e6; } }
🌟 Selected for report: WatchPug
Also found by: gzeon, toastedsteaksandwich
WatchPug
function flashloan(address token, address to, uint112 amount, bytes memory data) public lock { require(token == depositToken || token == rewardToken, "erc"); uint256 preDepositTokenBalance = ERC20(depositToken).balanceOf(address(this)); uint256 preRewardTokenBalance = ERC20(rewardToken).balanceOf(address(this)); ERC20(token).safeTransfer(to, amount); // the `to` contract should have a public function with the signature: // function lockeCall(address initiator, address token, uint256 amount, bytes memory data); LockeCallee(to).lockeCall(msg.sender, token, amount, data); uint256 postDepositTokenBalance = ERC20(depositToken).balanceOf(address(this)); uint256 postRewardTokenBalance = ERC20(rewardToken).balanceOf(address(this)); uint112 feeAmt = amount * 10 / 10000; // 10bps fee if (token == depositToken) { depositTokenFlashloanFeeAmount += feeAmt; require(preDepositTokenBalance + feeAmt <= postDepositTokenBalance, "f1"); require(preRewardTokenBalance <= postRewardTokenBalance, "f2"); } else { rewardTokenFeeAmount += feeAmt; require(preDepositTokenBalance <= postDepositTokenBalance, "f3"); require(preRewardTokenBalance + feeAmt <= postRewardTokenBalance, "f4"); } emit Flashloaned(token, msg.sender, amount, feeAmt); }
Given that only one token is related, reading the balanceOf of another token is unnecessary.
Change to:
function flashloan(address token, address to, uint112 amount, bytes memory data) public lock { require(token == depositToken || token == rewardToken, "erc"); uint256 preTokenBalance = ERC20(token).balanceOf(address(this)); ERC20(token).safeTransfer(to, amount); // the `to` contract should have a public function with the signature: // function lockeCall(address initiator, address token, uint256 amount, bytes memory data); LockeCallee(to).lockeCall(msg.sender, token, amount, data); uint256 postTokenBalance = ERC20(token).balanceOf(address(this)); uint112 feeAmt = amount * 10 / 10000; // 10bps fee if (token == depositToken) { depositTokenFlashloanFeeAmount += feeAmt; } else { rewardTokenFeeAmount += feeAmt; } require(preTokenBalance + feeAmt <= postTokenBalance, "fee"); emit Flashloaned(token, msg.sender, amount, feeAmt); }
#0 - 0xean
2022-01-17T12:53:00Z
I believe this to be a security measure, but will leave as valid so the sponsor can decide.
🌟 Selected for report: WatchPug
WatchPug
In Stream#claimReward(), ts.rewards is written 2 times and read once. Combing them into one storage write can save gas.
function claimReward() public lock { require(block.timestamp > endRewardLock, "lock"); TokenStream storage ts = tokensNotYetStreamed[msg.sender]; // accumulate reward per token info cumulativeRewardPerToken = rewardPerToken(); // update user rewards ts.rewards = earned(ts, cumulativeRewardPerToken); // update users last cumulative reward per token ts.lastCumulativeRewardPerToken = cumulativeRewardPerToken; lastUpdate = lastApplicableTime(); uint256 rewardAmt = ts.rewards; ts.rewards = 0; require(rewardAmt > 0, "amt"); // transfer the tokens ERC20(rewardToken).safeTransfer(msg.sender, rewardAmt); emit RewardsClaimed(msg.sender, rewardAmt); }
Change to:
function claimReward() public lock { require(block.timestamp > endRewardLock, "lock"); TokenStream storage ts = tokensNotYetStreamed[msg.sender]; // accumulate reward per token info cumulativeRewardPerToken = rewardPerToken(); uint256 rewardAmt = earned(ts, cumulativeRewardPerToken); require(rewardAmt > 0, "amt"); // update users last cumulative reward per token ts.lastCumulativeRewardPerToken = cumulativeRewardPerToken; lastUpdate = lastApplicableTime(); ts.rewards = 0; // transfer the tokens ERC20(rewardToken).safeTransfer(msg.sender, rewardAmt); emit RewardsClaimed(msg.sender, rewardAmt); }
🌟 Selected for report: WatchPug
WatchPug
currStreamId += 1;
Using ++currStreamId is more gas efficient than currStreamId += 1 for storage.
We suggest to use unchecked { ++currStreamId } to even better gas performance.
🌟 Selected for report: WatchPug
WatchPug
In Stream#updateStreamInternal(), the result of rewardPerToken() can be cached to avoid unnecessary storage reads.
// accumulate reward per token info cumulativeRewardPerToken = rewardPerToken(); // update user rewards ts.rewards = earned(ts, cumulativeRewardPerToken); // update users last cumulative reward per token ts.lastCumulativeRewardPerToken = cumulativeRewardPerToken;
Can be changed to:
// accumulate reward per token info uint256 _cumulativeRewardPerToken = rewardPerToken(); cumulativeRewardPerToken = _cumulativeRewardPerToken; // update user rewards ts.rewards = earned(ts, _cumulativeRewardPerToken); // update users last cumulative reward per token ts.lastCumulativeRewardPerToken = _cumulativeRewardPerToken;
// accumulate reward per token info cumulativeRewardPerToken = rewardPerToken(); // update user rewards ts.rewards = earned(ts, cumulativeRewardPerToken); // update users last cumulative reward per token ts.lastCumulativeRewardPerToken = cumulativeRewardPerToken;
Can be changed to:
// accumulate reward per token info uint256 _cumulativeRewardPerToken = rewardPerToken(); cumulativeRewardPerToken = _cumulativeRewardPerToken; // update user rewards ts.rewards = earned(ts, _cumulativeRewardPerToken); // update users last cumulative reward per token ts.lastCumulativeRewardPerToken = _cumulativeRewardPerToken;
🌟 Selected for report: WatchPug
WatchPug
function lockInternal() internal { require(unlocked == 1, "re"); unlocked = 2; } modifier lock { lockInternal(); _; unlocked = 1; }
lockInternal() is unnecessary as it's used only once. Therefore they can be inlined in lock to make the code simpler and save gas.
Change to:
modifier lock { require(unlocked == 1, "re"); unlocked = 2; _; unlocked = 1; }