Streaming Protocol contest - hyh's results

Handle

hyh

Vulnerability details

Impact

depositTokenFlashloanFeeAmount can be stolen with side effect of either rewardTokenFeeAmount or depositTokenAmount to be frozen within the contract.

If there are deposit token flash loan fees an attacker can wait for endDepositLock timestamp to pass and immediately run recoverTokens which doesn't account for depositTokenFlashloanFeeAmount and will allow attacker to withdraw all the flash loan fees accumulated for deposit tokens flash loans.

Side effect of this is that either

1. rewardTokenFeeAmount will be frozen within the contract as claimFees require both fee amounts, rewardTokenFeeAmount and depositTokenFlashloanFeeAmount to be transferred successfully, while the latter will be already gone, so transfer will fail on any run: https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L625

or

2. creatorClaimSoldTokens will fail in the similar manner on transfer attempt if claimFees was run first, retrieving depositTokenFlashloanFeeAmount at the expense of depositTokenAmount, which will be frozen in the contract this way: https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L597

Both (1) and (2) will lead to fund freeze because recoverTokens do account for depositTokenAmount and will not allow fund retrieval when the contract balance is less than that. Which variant will happen depends on what function be called first, correspondingly creatorClaimSoldTokens or claimFees.

Proof of Concept

depositTokenFlashloanFeeAmount isn't accounted in recoverTokens: https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L654

This way after endDepositLock timestamp pass anyone will be able to retrieve depositTokenFlashloanFeeAmount, which recoverTokens will treat as extra unaccounted funds.

Knowing this, an attacker will track the timestamp and run the transaction with highest economically viable gas price in the closest time slot, retrieving depositTokenFlashloanFeeAmount and causing either (1) or (2) scenario of deposit token funds freeze as a side effect (i.e. this is not grieving attack per se, but fund freeze comes a side effect as some part of the funds become missing, disabling the retrieval functionality).

Recommended Mitigation Steps

Add deposit flash loans fee to the recoverTokens logic (also including here the Sale case logic described in another issue posted earlier as this is an unrelated improvement to the same lines of code, i.e. 'to be' here is a merged version):

Now:

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

To be:

if (token == depositToken) {
	...
	uint256 accountedDepositTotal = depositTokenAmount;
	uint256 redeemedDepositTotal = isSale ? (claimedDepositTokens ? accountedDepositTotal : 0) : redeemedDepositTokens;
	uint256 excess = ERC20(token).balanceOf(address(this)) - (accountedDepositTotal - redeemedDepositTotal + depositTokenFlashloanFeeAmount);

Handle

hyh

Vulnerability details

Impact

Reward tokens accidently sent to the Stream contract cannot be recovered with recoverTokens if some reward tokens were already claimed with claimReward. As recoverTokens is the only recovering functionality in the contract the corresponding reward tokens will be frozen.

Proof of Concept

claimReward doesn't change rewardTokenAmount as other user's rewards are calculated based off it. But reward token balance of the Stream contract does change with each reward payoff: https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L575

This way reward tokens already claimed aren't accounted for in recoverTokens as only rewardTokenAmount is used there: https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L672

It looks like no fund stealing is possible here, the vulnerability only allows for freezing of the reward tokens accidently sent to the contract.

Recommended Mitigation Steps

Add claimedRewardTokens variable tracking cumulative reward tokens claimed

Now:

uint112 private rewardTokenAmount;
...
function claimReward() public lock {
	...
	uint256 rewardAmt = ts.rewards;
	...
	ERC20(rewardToken).safeTransfer(msg.sender, rewardAmt);

	emit RewardsClaimed(msg.sender, rewardAmt);
}
...
function recoverTokens(address token, address recipient) public lock {
...
	if (token == rewardToken) {
		...
		uint256 excess = ERC20(token).balanceOf(address(this)) - (rewardTokenAmount + rewardTokenFeeAmount);
		ERC20(token).safeTransfer(recipient, excess);
...

To be:

uint112 private rewardTokenAmount;
uint112 private claimedRewardTokens;
...
function claimReward() public lock {
	...
	uint256 rewardAmt = ts.rewards;
	...
	claimedRewardTokens += rewardAmt;
	ERC20(rewardToken).safeTransfer(msg.sender, rewardAmt);

	emit RewardsClaimed(msg.sender, rewardAmt);
}
...
function recoverTokens(address token, address recipient) public lock {
...
	if (token == rewardToken) {
		...
		uint256 excess = ERC20(token).balanceOf(address(this)) - (rewardTokenAmount - claimedRewardTokens + rewardTokenFeeAmount);
		ERC20(token).safeTransfer(recipient, excess);
...

Handle

hyh

Vulnerability details

Impact

Stream's recoverTokens will not allow recover any deposit tokens after Stream creator has claimed the depositTokenAmount of deposit tokens via creatorClaimSoldTokens in the sale Stream type case.

As there are no other token recovery mechanics the remaining funds, if any, will be effectively frozen within the contract.

Proof of Concept

When the Stream is a Sale after creatorClaimSoldTokens was successfully called the depositTokenAmount of depositToken will be transferred away: https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L583

Since this the recoverTokens logic will not work for depositToken as it will not allow withdrawing up to depositTokenAmount: https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L646

While there shouldn't be any deposit tokens left on the balance and so all extra deposit token should be withdrawable. This way any deposit tokens left will be frozen within the contract.

Recommended Mitigation Steps

Update the logic to account for creator's ability to claim deposit token stash when Stream type is Sale.

Now:

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

To be (adding extra memory variable to save on storage reads):

if (token == depositToken) {
	...
	uint256 accountedDepositTotal = depositTokenAmount;
	uint256 redeemedDepositTotal = isSale ? (claimedDepositTokens ? accountedDepositTotal : 0) : redeemedDepositTokens;
	uint256 excess = ERC20(token).balanceOf(address(this)) - (accountedDepositTotal - redeemedDepositTotal);

Handle

hyh

Vulnerability details

Impact

Any airdrop gathered with arbitraryCall will be immediately lost as an attacker can track arbitraryCall transactions and back run them with calls to recoverTokens, which doesn't track any tokens besides reward, deposit and incentive tokens, and will give the airdrop away.

Proof of Concept

arbitraryCall requires that tokens to be gathered shouldn't be reward, deposit or incentive tokens: https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L735

Also, the function doesn't mark gathered tokens in any way. Thus, the airdrop is freely accessible for anyone to be withdrawn with recoverTokens: https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L687

Recommended Mitigation Steps

Add airdrop tokens balance mapping, record what is gathered in arbitraryCall and prohibit their free withdrawal in recoverTokens similarly to incentives[].

Now:

mapping (address => uint112) public incentives;
...
function recoverTokens(address token, address recipient) public lock {
...
		if (incentives[token] > 0) {
			...
			uint256 excess = ERC20(token).balanceOf(address(this)) - incentives[token];
			...
		}

To be:

mapping (address => uint112) public incentives;
mapping (address => uint112) public airdrops;
...
function recoverTokens(address token, address recipient) public lock {
...
		if (incentives[token] > 0) {
			...
			uint256 excess = ERC20(token).balanceOf(address(this)) - incentives[token];
			...
		}
		if (airdrops[token] > 0) {
			...
			uint256 excess = ERC20(token).balanceOf(address(this)) - airdrops[token];
			...
		}
...
// we do know what airdrop token will be gathered
function arbitraryCall(address who, bytes memory data, address token) public lock externallyGoverned {
		...

		// get token balances
		uint256 preDepositTokenBalance = ERC20(depositToken).balanceOf(address(this));
		uint256 preRewardTokenBalance = ERC20(rewardToken).balanceOf(address(this));
		uint256 preAirdropBalance = ERC20(token).balanceOf(address(this));

		(bool success, bytes memory _ret) = who.call(data);
		require(success);
		
		uint256 postAirdropBalance = ERC20(token).balanceOf(address(this));
		require(postAirdropBalance <= type(uint112).max, "air_112");
		uint112 amt = uint112(postAirdropBalance - preAirdropBalance);
		require(amt > 0, "air");
		airdrops[token] += amt;