Streaming Protocol contest - gpersoon's results

Handle

gpersoon

Vulnerability details

Impact

The function recoverTokens() gives all excess tokens to the "streamCreator", however it doesn't take in account the fees for flashloans: "depositTokenFlashloanFeeAmount"

If you call claimFees() after you have called recoverTokens() then it will revert because the tokens for depositTokenFlashloanFeeAmount are no longer present. This means you also cannot claim the remaining rewardTokenFeeAmount tokens

Note: normally you would call claimFees() first and later on call recoverTokens(). However the flashloan functionally can keep accumulating depositTokenFlashloanFeeAmount and rewardTokenFeeAmount, as long as not everyone has called claimDepositTokens(). If you want to claim these via a second claimFees(), this will not work due to the difference between the remaining tokens and the administration of these tokens.

Proof of Concept

https://github.com/code-423n4/2021-11-streaming/blob/56d81204a00fc949d29ddd277169690318b36821/Streaming/src/Locke.sol#L696-L724

 function flashloan(address token, address to, uint112 amount, bytes memory data) public lock {
     ....
      if (token == depositToken) {
            depositTokenFlashloanFeeAmount += feeAmt;
            require(preDepositTokenBalance + feeAmt <= postDepositTokenBalance, "f1");
          ...

https://github.com/code-423n4/2021-11-streaming/blob/56d81204a00fc949d29ddd277169690318b36821/Streaming/src/Locke.sol#L646-L691

function recoverTokens(address token, address recipient) public lock {
         require(msg.sender == streamCreator, "!creator");
         if (token == depositToken) {
            ...
            uint256 excess = ERC20(token).balanceOf(address(this)) - (depositTokenAmount - redeemedDepositTokens);  // can claim too many tokens, also the depositTokenFlashloanFeeAmount tokens
            ERC20(token).safeTransfer(recipient, excess);
          ...
        }

https://github.com/code-423n4/2021-11-streaming/blob/56d81204a00fc949d29ddd277169690318b36821/Streaming/src/Locke.sol#L606-L630

 function claimFees(address destination) public lock externallyGoverned {
     ...
        fees = depositTokenFlashloanFeeAmount;
        if (fees > 0) {
            ..
            ERC20(depositToken).safeTransfer(destination, fees);  // will revert if there are not enough tokens left in the contract

Tools Used

Recommended Mitigation Steps

Account for depositTokenFlashloanFeeAmount in the function recoverTokens()

Handle

gpersoon

Vulnerability details

Impact

The contract Stream uses 2 tokens: depositToken and rewardToken, which are specified in the constructor. Suppose (accidentally) the same addresses are supplied, such that depositToken == rewardToken. Most of the code will still work, however the function recoverTokens() will give problems.

It sends excess tokens to the streamCreator, but when depositToken == rewardToken then: If you retrieve the excess "depositToken"s, you will get the tokens reserved as rewardToken If you retrieve the excess "rewardToken"s, you will get the tokens reserved as depositToken

Depending on the endDepositLock and endRewardLock time you will enter one or two of the if statements.

Ergo it is safer to make sure depositToken!= rewardToken as it doesn't seem logical to use the same token for depositToken and rewardToken.

Proof of Concept

https://github.com/code-423n4/2021-11-streaming/blob/56d81204a00fc949d29ddd277169690318b36821/Streaming/src/Locke.sol#L263-L311

contract Stream is LockeERC20, ExternallyGoverned {
...
constructor(....        address _rewardToken,  address _depositToken...) {
...
// set tokens
        depositToken = _depositToken;
        rewardToken = _rewardToken;
}

https://github.com/code-423n4/2021-11-streaming/blob/56d81204a00fc949d29ddd277169690318b36821/Streaming/src/Locke.sol#L646-L691

function recoverTokens(address token, address recipient) public lock {
        require(msg.sender == streamCreator, "!creator");
        if (token == depositToken) {
            require(block.timestamp > endDepositLock, "time");
            uint256 excess = ERC20(token).balanceOf(address(this)) - (depositTokenAmount - redeemedDepositTokens); // will get too many tokens if depositToken==rewardToken
            ERC20(token).safeTransfer(recipient, excess);
           ..
...        }       
        if (token == rewardToken) {
            require(block.timestamp > endRewardLock, "time");
            uint256 excess = ERC20(token).balanceOf(address(this)) - (rewardTokenAmount + rewardTokenFeeAmount); // will get too many tokens if depositToken==rewardToken
            ERC20(token).safeTransfer(recipient, excess);
            ....
        }

Tools Used

Recommended Mitigation Steps

Check if there is a usecase for haveing depositToken and rewardToken as the same token. If so, adapt recoverTokens. Otherwise add something like to following to the constructor: require(depositToken != rewardToken,"The same");

Handle

gpersoon

Vulnerability details

Impact

Suppose someone stakes some tokens and then withdraws all of his tokens (he can still withdraw). This will result in ts.tokens being 0.

Now after some time he stakes some tokens again. At the second stake updateStream() is called and the following if condition is false because ts.tokens==0

  if (acctTimeDelta > 0 && ts.tokens > 0) {

Thus ts.lastUpdate is not updated and stays at the value from the first withdraw. Now he does a second withdraw. updateStream() is called an calculates the updated value of ts.tokens. However it uses ts.lastUpdate, which is the time from the first withdraw and not from the second stake. So the value of ts.token is calculated incorrectly. Thus more tokens can be withdrawn than you are supposed to be able to withdraw.

Proof of Concept

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

function stake(uint112 amount) public lock updateStream(msg.sender) {
      ...         
        uint112 trueDepositAmt = uint112(newBal - prevBal);
       ... 
        ts.tokens += trueDepositAmt;

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

function withdraw(uint112 amount) public lock updateStream(msg.sender) {
        ...
        ts.tokens -= amount;

https://github.com/code-423n4/2021-11-streaming/blob/56d81204a00fc949d29ddd277169690318b36821/Streaming/src/Locke.sol#L203-L250

function updateStreamInternal(address who) internal {
...
 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);
            }

Tools Used

Recommended Mitigation Steps

Change the code in updateStream() to:

    if (acctTimeDelta > 0 ) {
                // some time has passed since this user last interacted
                // update ts not yet streamed
                if (ts.tokens > 0) 
                      ts.tokens -= uint112(acctTimeDelta * ts.tokens / (endStream - ts.lastUpdate));
                ts.lastUpdate = uint32(block.timestamp);  // always update ts.lastUpdate (if time has elapsed)
            }

Note: the next if statement with unstreamed and lastUpdate can be changed in a similar way to save some gas

Handle

gpersoon

Vulnerability details

Impact

The function stake() increases unstreamed, however the function withdraw(), that does the inverse of stake() doesn't decrease unstreamed. The function withdraw() does update all the other relevant variables so this seems to be an omission.

Thus the value of unstreamed will not be accurate after a withdraw(). It doesn't seem to have negative consequences for the protocol, but the user interface & management interfaces will show misleading values.

Proof of Concept

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

function stake(uint112 amount) public lock updateStream(msg.sender) {
...
        unstreamed += trueDepositAmt;

function withdraw(uint112 amount) public lock updateStream(msg.sender) {
    // unstreamed not updated

Tools Used

Recommended Mitigation Steps

Update unstreamed in withdraw()