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: 8/36
Findings: 5
Award: $4,548.09
🌟 Selected for report: 4
🚀 Solo Findings: 0
kenzo
Only the governance should be able to collect flash loan fees, however due to a bug, the stream creator can claim deposit token flashloan fees too.
Lost fees for the protocol.
When a flash loan of deposit token is being made, and fees are added to the contract's depositToken balance, Streaming increases depositTokenFlashloanFeeAmount :
depositTokenFlashloanFeeAmount += feeAmt;
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L714
However, in recoverTokens, Streaming does not substract that amount from the amount that is rescueable:
uint256 excess = ERC20(token).balanceOf(address(this)) - (depositTokenAmount - redeemedDepositTokens); ERC20(token).safeTransfer(recipient, excess);
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L654 Therefore, the deposit token flashloan fees would be considered as "excess" and the stream creator could claim them.
Include depositTokenFlashloanFeeAmount in the excess calculation above.
#0 - 0xean
2022-01-14T20:53:51Z
dupe of #241
🌟 Selected for report: cmichel
Also found by: GeekyLumberjack, hyh, kenzo, pedroais
kenzo
In claimReward, Streaming does not save the amount of rewards claimed.
This leads to wrong excess calculation in recoverTokens which will underflow the calculation and revert the rescue attempt if enough rewards have been claimed.
Rescuing of reward tokens will mostly not work if we assume people will claim their rewards.
I classified this issue as High Risk because of few reasons:
recoverTokens functionality is not implemented in all protocols, but here it gives the users this functionality/assurance.
Therefore, since the functionality is not working I consider it as a case of lost user funds.
However, since the loss of users funds relates to accidentally sent tokens and not protocol funds, perhaps this issue should be downgraded to medium. I am not sure, but because of the abovementioned reasons I classified it as high.When a stream is being funded, the funded amount is saved to rewardTokenAmount:
rewardTokenAmount += amt;
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L403 (also #L406) And when reward tokens are being rescued, this amount is removed from excess, so to not remove funds which belong to users:
uint256 excess = ERC20(token).balanceOf(address(this)) - (rewardTokenAmount + rewardTokenFeeAmount);
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L672
However, when users are claiming rewards, the stream's rewardToken balance will decrease but rewardTokenAmount is not updated to reflect it. It is not changed at all.
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L555:#L578
Therefore in the abovementioned excess calculation:
uint256 excess = ERC20(token).balanceOf(address(this)) - (rewardTokenAmount + rewardTokenFeeAmount);
(let's assume rewardTokenFeeAmount = 0 for simplicity)
If people have claimed more rewards than tokens that need to be rescued, then ERC20(token).balanceOf(address(this)) < rewardTokenAmount, the calculation will underflow and the rescue attempt will revert.
If I am not mistaken, rewardTokenAmount can be decreased in claimReward.
Alternatively, create another variable rewardTokenClaimed, increase it in claimReward, and remove it from the excess in recoverTokens.
#0 - ninek9
2021-12-06T18:21:24Z
ADDITIONAL COMMENT FROM WARDEN:
Note: throughout this issue when I wrote "rescue reward tokens", I meant "rescue excess rewardToken tokens". Obviously the reward tokens themselves shouldn't be rescued, only the excess.
#1 - 0xean
2022-01-14T21:14:38Z
Dupe of #214
🌟 Selected for report: harleythedog
kenzo
If the stream is a sale, after the creator has claimed the deposit tokens, Streaming does not save the amount of tokens claimed. Therefore if trying to rescue excess depositToken tokens, the calculation will underflow and the rescue attempt will revert and fail.
Rescuing of depositToken tokens does not work if the stream is a sale and the deposit tokens have been claimed.
I classified this issue as High Risk because of few reasons:
recoverTokens functionality is not implemented in all protocols, but here it gives the users this functionality/assurance.
Therefore, since the functionality is not working I consider it as a case of lost user funds.
However, since the loss of users funds relates to accidentally sent tokens and not protocol funds, perhaps this issue should be downgraded to medium. I am not sure, but because of the abovementioned reasons I classified it as high.When users stake their depositToken tokens, the amount is saved to depositTokenAmount:
depositTokenAmount += trueDepositAmt;
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L431
When not in a sale, when users claim their deposit tokens, the amount redeemed is saved in redeemedDepositTokens:
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L544
In recoverTokens, when calculating the amount of depositToken tokens that can be rescued, Streaming removes the amount of deposit tokens which have been redeemed by depositors:
uint256 excess = ERC20(token).balanceOf(address(this)) - (depositTokenAmount - redeemedDepositTokens);
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L654
However, this does not take into account the amount of depositToken tokens which have been redeemed by the creator in the case of a sale. In that case, in creatorClaimSoldTokens, there is nothing that updates the amount of tokens that have been claimed:
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L583:#L600
Therefore, the abovementioned excess calculation:
uint256 excess = ERC20(token).balanceOf(address(this)) - (depositTokenAmount - redeemedDepositTokens);
Will underflow if the creator has claimed sold deposits and the amount of tokens needing to be rescued is smaller than that.
Either decrease depositTokenAmount in creatorClaimSoldTokens,
Or, save there the amount of tokens the creator has claimed, and substract that from the excess calculation above.
#0 - brockelmore
2021-12-08T22:22:10Z
ignore above mention ^
#1 - 0xean
2022-01-15T01:26:58Z
dupe of #121
kenzo
In StreamFactory, the protocol max fee is defined to be 500. However in Stream, it is checked whether the protocol fee is < 10000 (total basis points).
I think this might be considered "function incorrect as to spec" and this is why I rated this issue low severity. At the very least this issue can raise some confusion when reading the code.
In StreamFactory, max fee is hardcoded to be 500: ` `` uint16 constant MAX_FEE_PERCENT = 500; // 500/10000 == 5%
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L773 And upon update of fee, it is checked against that variable:
require(newFeeParams.feePercent <= MAX_FEE_PERCENT, "fee");
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L851 In Stream, the fee percent is limited to 10000.
require(feePercent < 10000, "fee");
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L286 ## Recommended Mitigation Steps I suggest having the two values match - either by reading the max fee from the factory, or by hardcoding it to match.
#0 - brockelmore
2022-01-05T17:35:48Z
duplicate #246
🌟 Selected for report: kenzo
kenzo
The comment in claimReward says:
Allows a receipt token holder (or original depositor in case of a sale) to claim their rewardTokens
However, the function only allows the original depositor to claim the rewards. Additionally, the C4 readme doesn't mention that the receipt token holder should be able to withdraw the reward.
Confusion regarding working of protocol.
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L553
Change the comment to reflect the fact that only original depositor may withdraw rewards.
🌟 Selected for report: kenzo
kenzo
unstreamed (global unstreamed) is not updated on withdrawal.
As far as I see it is not used in calculations, so only wrong information presented to users.
Upon user staking, the global unstreamed amount is increased:
unstreamed += trueDepositAmt
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L438
This value is kept up to date in updateStreamInternal:
uint256 globalStreamingSpeedPerSecond = (uint256(unstreamed) * 10**6)/ (endStream - lastUpdate); unstreamed -= uint112((uint256(tdelta) * globalStreamingSpeedPerSecond) / 10**6);
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L236:#L239
However, when a user withdraws funds via withdraw (/exit), there is no decrease of unstreamed:
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L455:#L479
Therefore, unstreamed will return more tokens than truly are.
I believe the following line should be added to withdraw:
unstreamed -= amount;
18.2633 USDC - $18.26
kenzo
exit function has updateStream modifier, which is unnecessary because basically all exit does is call withdraw which already has updateStream modifier.
Some gas can be saved. Second run of modifier will just do some checks and nothing is needed to be changed.
exit has the updateStream modifier and only calls withdraw:
function exit() public updateStream(msg.sender) { TokenStream storage ts = tokensNotYetStreamed[msg.sender]; uint112 amount = ts.tokens; withdraw(amount); }
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L487:#L493
withdraw already has that modifier:
function withdraw(uint112 amount) public lock updateStream(msg.sender) {
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L455 So the second run of the modifier is unnecessary and will just waste gas.
Remove the modifier from exit.
#0 - ninek9
2021-12-07T01:49:29Z
ADDITIONAL COMMENT FROM WARDEN:
On further thought, I realize that if we don't have the modifier on the exit function, then ts.tokens wouldn't get updated, and we'll send the wrong amounts to withdraw. So I think the issue is invalid but not sure.
#1 - 0xean
2022-01-17T13:52:33Z
dupe of #187
🌟 Selected for report: kenzo
100.2104 USDC - $100.21
kenzo
Since claimReward can only be called after endRewardLock, lastApplicableTime will always return endStream.
Some gas can be saved.
claimReward will only run if time > endRewardLock (which is >= endStream):
require(block.timestamp > endRewardLock, "lock");
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L556
claimReward is calling lastApplicableTime:
lastUpdate = lastApplicableTime();
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L567
And this is lastApplicableTime:
return block.timestamp <= endStream ? uint32(block.timestamp) : endStream;
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L340
Therefore, it will always return endStream.
In claimReward, change this line:
lastUpdate = lastApplicableTime();
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L340 To:
lastUpdate = endStream;
🌟 Selected for report: kenzo
kenzo
In updateStreamParams and updateFeeParams, the old values are being saved (probably in order to log them) before being updated.
This is unnecessary.
Small amount of gas can be saved. (Although maybe it was done this way for code clarity..?)
In updateFeeParams
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L850:#L855:
function updateFeeParams(GovernableFeeParams memory newFeeParams) public governed { require(newFeeParams.feePercent <= MAX_FEE_PERCENT, "fee"); GovernableFeeParams memory old = feeParams; feeParams = newFeeParams; emit FeeParametersUpdated(old, newFeeParams); }
The code can be changed to:
function updateFeeParams(GovernableFeeParams memory newFeeParams) public governed { require(newFeeParams.feePercent <= MAX_FEE_PERCENT, "fee"); emit FeeParametersUpdated(feeParams, newFeeParams); feeParams = newFeeParams; }
Same thing can be done in updateStreamParams.
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L841:#L848