Flow

Summary

The recovery functionality, while intended to allow the recovery of surplus tokens only, can be bypassed, allowing deposited tokens to be withdrawn in the case of multi-address ERC-20 tokens.

The admin's ability to withdraw deposited tokens undermines the protocol's purpose of being trustless, and more importantly, it may lead to its insolvency.

Vulnerability Details

As can be seen from the code, the admin should be able to recover only the unaccounted tokens (i.e., those sent directly to the contract by mistake):

https://github.com/Cyfrin/2024-10-sablier/blob/main/src/abstracts/SablierFlowBase.sol#L231

The same is mentioned in the README.md:

1. Protocol Admin: Protocol admin has the ability to set protocol fee, collect protocol revenue, recover surplus tokens and set NFT descriptor.

However, the "surplus logic" can be bypassed, allowing tokens deposited into the stream balance to be withdrawn in the case of ERC-20 tokens with multiple entry points. These tokens have more than one contract address that can manipulate the same storage variables. The most notable examples of such tokens are SNX and sBTC.

The issue could arise in either of these cases:

• Malicious admin action

• Unintentional mistake, whether genuine or caused by a social engineering attack

Please consider the following simple example:

• A user creates a stream and deposits 1000 SNX using main (proxy) address of SNX: https://etherscan.io/address/0xc011a73ee8576fb46f5e1c5751ca3b9fe0af2a6f

  • if recover called with main SNX address, it will reverts since token.balanceOf is 1000 and aggregateBalance[token] is 1000

• A malicious admin calls recover supplying the underlying address of SNX as token parameter: https://etherscan.io/address/0xc5f0b4194455e0C175ab68C501400e46C7203504

  • now, token.balanceOf will still return 1000 but aggregateBalance is zero hence the surplus calculated as 1000 tokens, thus recover will succeed and transfer the whole balance out of the contract.

As can be seen from the example above, the root cause of the issue is that aggregateBalance relies on the token address, while in the case of SNX, there are two addresses.

Impact

The impact is high, as it may lead to the entire balance of the token being withdrawn. The likelihood is low, since it requires malicious or misled admin action and will work only with rare, specific tokens. Thus, the severity is medium.

Tools Used

Manual review

Recommendations

There is no straightforward fix. The best solution seems to be to remove the recovery functionality. Alternatively, you could explicitly warn in the protocol documentation that such tokens are not supported.