recoverTokens doesn't work when isSale is true

[code423n4]

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.

[brockelmore]

duplicates: #180, #268, #88

[0xean]

upgrading to High as assets would be lost in the case outlined by the warden

3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).