Streaming Protocol contest - harleythedog's results

Handle

harleythedog

Vulnerability details

Impact

In the case where recoverTokens is called with token == depositToken, the calculation for the excess number of deposit tokens in the contract is:

uint256 excess = ERC20(token).balanceOf(address(this)) - (depositTokenAmount - redeemedDepositTokens);

This does not account for the amount of deposit tokens in the contract due to flashloan fees, which is recorded in the variable depositTokenFlashloanFeeAmount. As a result, a recoverTokens call made by the stream creator to a user that needs tokens recovered would send MORE tokens than the user actually deserves. The function would send all depositTokenFlashloanFeeAmount deposit tokens to the user, which could be a very large amount due to the nature of flashloans.

Due to the fact that this could be such a large mistake, I believe this to be a high severity issue. An attacker could send a small amount of deposit tokens to the contract intentionally, and then act like it was a mistake. It would be hard to spot the bug that the recovery would also send all of the flashloan fees to the attacker as well, so it is a very real possibility that the stream creator would fall for this trick. At the very least, the function in its current state is unusable because of this bug.

Proof of Concept

See the calculation of excess deposit tokens here: https://github.com/code-423n4/2021-11-streaming/blob/56d81204a00fc949d29ddd277169690318b36821/Streaming/src/Locke.sol#L654

Tools Used

Inspection

Recommended Mitigation Steps

Change the calculation to instead be:

uint256 excess = ERC20(token).balanceOf(address(this)) - (depositTokenAmount + depositTokenFlashloanFeeAmount - redeemedDepositTokens);

Handle

harleythedog

Vulnerability details

Impact

In recoverTokens, the logic to calculate the excess number of deposit tokens in the contract is:

uint256 excess = ERC20(token).balanceOf(address(this)) - (depositTokenAmount - redeemedDepositTokens);

This breaks in the case where isSale is true and the deposit tokens have already been claimed through the use of creatorClaimSoldTokens. In this case, redemeedDepositTokens will be zero, and depositTokenAmount will still be at its original value when the streaming ended. As a result, any attempts to recover deposit tokens from the contract would either revert or send less tokens than should be sent, since the logic above would still think that there are the full amount of deposit tokens in the contract. This breaks the functionality of the function completely in this case.

Proof of Concept

See the excess calculation here: https://github.com/code-423n4/2021-11-streaming/blob/56d81204a00fc949d29ddd277169690318b36821/Streaming/src/Locke.sol#L654

See creatorClaimSoldTokens here: https://github.com/code-423n4/2021-11-streaming/blob/56d81204a00fc949d29ddd277169690318b36821/Streaming/src/Locke.sol#L583

Notice that creatorClaimSoldTokens does not change depositTokenAmount or redeemedDepositTokens, so the excess calculation will be incorrect in the case of sales.

Tools Used

Inspection

Recommended Mitigation Steps

I would recommend setting redeemedDepositTokens to be depositTokenAmount in the function creatorClaimSoldTokens, since claiming the sold tokens is like "redeeming" them in a sense. This would fix the logic issue in recoverTokens.

Handle

harleythedog

Vulnerability details

Impact

The storage variable unstreamed keeps track of the global amount of deposit token in the contract that have not been streamed yet. This variable is a public variable, and users that read this variable likely want to use its value to determine whether or not they want to stake in the stream.

The issue here is that unstreamed is incremented on calls to stake, but it is not being decremented on calls to withdraw. As a result, a malicious user could simply stake, immediately withdraw their staked amount, and they will have increased unstreamed. They could do this repeatedly or with large amounts to intentionally inflate unstreamed to be as large as they want.

Other users would see this large amount and be deterred to stake in the stream, since they would get very little reward relative to the large amount of unstreamed deposit tokens that appear to be in the contract. This benefits the attacker as less users will want to stake in the stream, which leaves more rewards for them.

Proof of Concept

See stake here: https://github.com/code-423n4/2021-11-streaming/blob/56d81204a00fc949d29ddd277169690318b36821/Streaming/src/Locke.sol#L417

See withdraw here: https://github.com/code-423n4/2021-11-streaming/blob/56d81204a00fc949d29ddd277169690318b36821/Streaming/src/Locke.sol#L455

Notice that stake increments unstreamed but withdraw does not affect unstreamed at all, even though withdraw is indeed removing unstreamed deposit tokens from the contract.

Tools Used

Inspection

Recommended Mitigation Steps

Add the following line to withdraw to fix this issue:

unstreamed -= amount;