Sablier Flow 1.2.0

Description

The campaign admin is allowed to claw back the funds if the first claim hasn't happened yet. In Merkle VCA campaigns, the first claim is more likely to occur near or after the end time since users are incentivized to do so. There may be a potential rug-pull scenario if the admin is malicious:

1. The admin funds the VCA campaign with sufficient funds.
2. All users wait until the end time as they believe they will be able to receive the full amount.
3. The admin claws back all the funds right before the end time (or before the first claim).
4. The users end up receiving nothing in return.

The above scenario is more of a concern for VCA campaigns due to its design of encouraging users to claim late, which increases the attack's likelihood and success rate.

Recommendation

A possible mitigation is to disable the grace period of VCA campaigns, thus preventing the campaign admin from clawing back the funds during the campaign period but only when the campaign has expired.

Sablier: Thank you for your submission. While we recognize that clawback could potentially be misused in the case of Merkle VCA, we also believe that a grace period is necessary for addressing issues a misconfigured campaign, such as an incorrect Merkle root.

That said, your concern is completely valid. To address this, we plan to introduce a dummy eligible recipient, perhaps with a value of just 1 wei. This recipient can be used to test the claim process immediately after the campaign is created. Not only will this ensure the claim is functioning correctly, but it will also trigger the grace period.

Cantina Managed: Acknowledged.

Description

In SablierMerkleLT, the validation of all parameters is deferred until the first Stream is created. For example, the timestamps of the tranches are only validated in the Lockup contract.

Therefore, at L123, endTime is not yet validated and can be incorrect. A creator may accidentally provide some incorrect parameters that result in an incorrect endTime and less than the current time, therefore unlocking all the airdrops at once unexpectedly. Before the latest changes, such incorrect parameters would be rejected when creating the first Stream, so the creator would have a chance to claw back the funds and create a new campaign.

A similar issue also applies to SablierMerkleLL, where a creator may accidentally provide a cliffDuration less than the totalDuration, resulting in the end time being earlier than the cliff time.

Recommendation

The design choice of deferring the parameter checks until the first user claim is to allow the campaign creator to claw back the pre-funded tokens in case the parameters are invalid. However, with the latest changes, validating the timestamps becomes necessary even before the first claim.

The issue can be fixed by checking the timestamps in the constructor (when the airdrop campaigns are created). This would be a reasonable tradeoff given that there haven't been any pre-funded campaigns seen in practice at the time of writing, confirmed by the protocol team. In addition to the code changes, consider adding a warning in the docs that campaign creators should not pre-fund the campaigns before deploying them.

Sablier: Thanks for the finding. We have decided to take no action on this.

The grace period and the sender's ability to clawback funds are sufficient to mitigate this problem. Even if we implement all the checks in the constructor, it will not be a complete solution since start time can be set as block.timestamp in case of non-ranged streams.

See the full discussion here.

Cantina Managed: It is correct that adding checks in the constructor is not sufficient for non-ranged streams, since the start time can be set to block.timestamp, and therefore the end time needs to be calculated and checked at claim time (e.g., by removing the unchecked block, if a mitigation is implemented. Acknowledged.

Description

Using token.safeTransferFrom() from the SafeERC20 library is necessary at L422. It is because transferFrom() cannot handle ERC-20 tokens that do not return a boolean (e.g., USDT on Ethereum mainnet). In that case, the call to transferFrom() will revert. Since the function is to recover any accidentally transferred ERC-20 tokens, it would be better to make the function compatible with those ERC-20 tokens that do not follow the standard.

Recommendation

Consider using the safeTransferFrom() function from SafeERC20.

Sablier: Fixed in PR 405.

There was indeed an issue with tokens like USDT. We’re now using the safe version, and we’ve renamed it to transferTokens to avoid confusion with ERC721.transferFrom.

Cantina Managed: Verified.

Description

The SablierMerkleBase contract reads the native token price from a Chainlink price feed and calculates the minimum fee required for each airdrop claim. Although the code handles the case of invalid prices (which are less than or equal to 0), the prices can still be stale since the updatedAt value returned from the oracle is not checked against the current timestamp.

As a result, there could be a scenario where a user is paying enough fees from the UI (calculated based on the market price), but because of the stale price from the oracle, the transaction fails due to the minimum fee check. The user would then have to pay more than the minimum fee, which, however, could discourage small claims as they may become less profitable.

For example, assuming a minimum fee of 1 USD is set at the contract level, and for a claim of 5 USD, the fee charged on the UI is 1.1 USD. With such assumptions, the market price can only be at most 10% higher than the oracle price. If it exceeds 10%, then the user will need to pay more than 1.1 USD when claiming.

Recommendation

Consider checking if the price from the oracle is stale, and if so, return 0 from _minimumFeeInWei() to disable the fees temporarily.

A stale price can be defined as block.timestamp - updatedAt > heartbeat + delta. For example, for the ETH/USD price feed on Ethereum mainnet, the heartbeat is 3600 seconds. The delta parameter allows additional time for the price-updating transaction to be included on-chain (in case network congestion happens, etc.) For reference, here's Chainlink's example of reading a price feed and handling stale prices with a fallback price.

To be flexible and account for different heartbeat values across chains and price feeds, the total heartbeat + delta can be set as a configurable delay variable in the factory contract. The protocol admin can then adjust the delay settings if needed. Alternatively, a custom Oracle contract can be deployed on each chain, which includes stale price checks. Similarly, the protocol admin can set its delay value or make delay an immutable variable.

Sablier: Agree on the finding. Fixed in PR 93.

Cantina Managed: Verified.

Description

In the current design, the airdrop campaigns only support ERC-20-compliant tokens instead of native tokens. Native tokens are charged as protocol fees in each claim. Such a design could cause issues if the native token of the chain has an ERC-20 representation and is used as the campaign token. If so, the protocol would fail to distinguish between the deposited ERC-20 tokens and the received protocol fees.

For example, CELO, the native token of the Celo blockchain, has an ERC-20 representation at address 0x471EcE3750Da237f93B8E339c536989b8978a438. By design, calling CELO.transfer() has the same effect as transferring CELO with msg.value, and vice versa. In case an airdrop campaign has a TOKEN of CELO, there would be two unexpected outcomes:

1. If the campaign admin can claw back the funds, they can also withdraw the protocol fees kept in the contract. Note that the admin can also withdraw the protocol fees during the claim period by submitting a valid Merkle proof.
2. Anyone calling factory.collectFees() would accidentally transfer all the held CELO tokens from the campaign to the factory admin. It is because in collectFees(), the entire address(this).balance is transferred.

Below are some other blockchains whose native token also has an ERC-20 representation:

• Polygon: 0x0000000000000000000000000000000000001010
• Metis: 0xDeadDeAddeAddEAddeadDEaDDEAdDeaDDeAD0000
• Moonbeam: 0x0000000000000000000000000000000000000802
• Tangle: 0x0000000000000000000000000000000000000802

It is worth noting that not all ERC-20 representations listed above fully comply with the ERC-20 standard. For example, the POL token on Polygon does not implement a transferFrom() or approve() function.

Recommendation

A possible solution could be keeping track of the deposited ERC-20 tokens and the charged protocol fees in separate state variables and enforcing the parties to withdraw the token up to the tracked balances. This would avoid confusion between the deposited tokens and the protocol fees.

Alternatively, consider adding a warning in the docs that users on the affected chains should avoid using the ERC-20 representation of the native token for airdrop campaigns.

Sablier: Fixed in PR 93.

Great finding. Instead of tracking fees charged in a separate variable, we have decided to enable an option to block native tokens from being used with the protocol, if they implement an interface similar to ERC-20.

The rationale is that because this is only applicable on very few chains, adding new variables to track fees would impact the experience of non-affected chains, which are large in number.

Cantina Managed: Verified. The admin should be responsible for setting the correct ERC-20 representations on applicable chains.

Description

In the current design, the Lockup contract only supports ERC-20-compliant tokens instead of native tokens. Native tokens, as protocol fees, are charged on the UI for specific operations. Such a design could cause issues if the native token of the chain has an ERC-20 representation and is used as the token of a Stream. If so, the protocol would fail to distinguish between the deposited ERC-20 tokens and the received protocol fees.

For example, CELO, the native token of the Celo blockchain, has an ERC-20 representation at address 0x471EcE3750Da237f93B8E339c536989b8978a438. By design, calling CELO.transfer() has the same effect as transferring CELO with msg.value, and vice versa.

As a result, anyone calling collectFees() on the Lockup contract would accidentally transfer all the CELO tokens in the contract to the admin. This is because in collectFees(), the entire address(this).balance is transferred. As those CELO tokens are deposited for some Streams, this would affect the withdrawals of the Stream recipients.

Below are some other blockchains whose native token also has an ERC-20 representation:

• Polygon: 0x0000000000000000000000000000000000001010
• Metis: 0xDeadDeAddeAddEAddeadDEaDDEAdDeaDDeAD0000
• Moonbeam: 0x0000000000000000000000000000000000000802
• Tangle: 0x0000000000000000000000000000000000000802

It is worth noting that not all ERC-20 representations listed above fully comply with the ERC-20 standard. For example, the POL token on Polygon does not implement a transferFrom() or approve() function.

Recommendation

A possible solution could be keeping track of the deposited ERC-20 tokens and the charged protocol fees in separate state variables and enforcing the parties to withdraw the token up to the tracked balances. This would avoid confusion between the deposited tokens and the protocol fees.

Alternatively, consider adding a warning in the docs that users on the affected chains should avoid using the ERC-20 representation of the native token for Lockup Streams.

Sablier: Fixed in PR 1209.

Great finding. Instead of tracking fees charged in a separate variable, we have decided to enable an option to block native tokens from being used with the protocol, if they implement an interface similar to ERC-20.

The rationale is that because this is only applicable on very few chains, adding new variables to track fees would impact the experience of non-affected chains, which are large in number.

Cantina Managed: Verified. The admin should be responsible for setting the correct ERC-20 representations on applicable chains.

Description

In the current design, the Flow contract only supports ERC-20-compliant tokens instead of native tokens. Native tokens, as protocol fees, are charged on the UI for specific operations. Such a design could cause issues if the native token of the chain has an ERC-20 representation and is used as the token of a Flow. If so, the protocol would fail to distinguish between the deposited ERC-20 tokens and the received protocol fees.

For example, CELO, the native token of the Celo blockchain, has an ERC-20 representation at address 0x471EcE3750Da237f93B8E339c536989b8978a438. By design, calling CELO.transfer() has the same effect as transferring CELO with msg.value, and vice versa.

As a result, anyone calling collectFees() on the Flow contract would accidentally transfer all the CELO tokens in the contract to the admin. This is because in collectFees(), the entire address(this).balance is transferred. As those CELO tokens are deposited for some Flows, this would affect the withdrawals of the Flow recipients.

Below are some other blockchains whose native token also has an ERC-20 representation:

• Polygon: 0x0000000000000000000000000000000000001010
• Metis: 0xDeadDeAddeAddEAddeadDEaDDEAdDeaDDeAD0000
• Moonbeam: 0x0000000000000000000000000000000000000802
• Tangle: 0x0000000000000000000000000000000000000802

It is worth noting that not all ERC-20 representations listed above fully comply with the ERC-20 standard. For example, the POL token on Polygon does not implement a transferFrom() or approve() function.

Recommendation

A possible solution could be keeping track of the deposited ERC-20 tokens and the charged protocol fees in separate state variables and enforcing the parties to withdraw the token up to the tracked balances. This would avoid confusion between the deposited tokens and the protocol fees.

Alternatively, consider adding a warning in the docs that users on the affected chains should avoid using the ERC-20 representation of the native token for Flows.

Sablier: Fixed in PR 405.

Great finding. Instead of tracking fees charged in a separate variable, we have decided to enable an option to block native tokens from being used with the protocol, if they implement an interface similar to ERC-20.

The rationale is that because this is only applicable on very few chains, adding new variables to track fees would impact the experience of non-affected chains, which are large in number.

Cantina Managed: Verified. The admin should be responsible for setting the correct ERC-20 representations on applicable chains.

Description

1. SablierMerkleFactoryBase.sol#L83-L86: Consider updating the comment to

   // Effect: enable custom fee for the user.

   to match the actual behavior. Also, consider moving this if block after the next if block (which checks the newFee variable) to follow the Checks-Effects-Interactions (CEI) pattern.

2. ISablierMerkleBase.sol#L87: Consider updating the comment to

   The msg.value must not be less than minimumFeeInWei

3. SablierMerkleVCA.sol#L113: Consider updating the comment to

   // Check: unlock start time is not in the future or now

   Since a >= is used instead of >, users cannot claim at the start time.

4. DataTypes.sol#L130: Consider removing the second sentence, "A value of zero means the campaign does not expire", since Merkle VCA campaigns should expire eventually by having a non-zero expiration time.

The following issues are identified from at commit dc6bbd6:

1. ISablierMerkleVCA.sol#L45: Consider adding the following to the calculateForgoneAmount() function's NatSpec:

   Returns zero if the claim time is less than the vesting start time. Since the claim cannot be made, no amount can be forgone.

2. SablierMerkleVCA.sol#L154-L156: In _calculateClaimAmount(), the block if (claimTime == VESTING_START_TIME) { ... } is not necessary. Considering most of the claims happen after the start time, removing this if block could save gas for most users. However, it can be kept to be explicit.

3. airdrops/SECURITY.md: Consider adding the following warning for airdrop campaigns:

   Do not pre-fund an airdrop campaign (i.e., sending tokens to the to-be-deployed address) before creating it through the factory. Otherwise, if the provided parameters are invalid, the campaign will fail to be created, and the funds will be lost.

4. Consider following the same event emission syntax across all the protocol contracts. As an example, in SablierFactoryMerkleBase.sol#L65-L79:

   • In collectFees function an event is emitted as emit CollectFees(admin, campaign, feeAmount);
   • In disableCustomFeeUSD function an event is emitted as emit DisableCustomFeeUSD({ admin: msg.sender, campaignCreator: campaignCreator });

Recommendation

Consider implementing the above suggestions.

Sablier: Fixed in PR 93 and PR 140.

Cantina Managed: Verified.

Description

1. ISablierFlow.sol: All the non-view functions defined in the ISablierFlow interface, except transferFrom(), emits a MetadataUpdate() event. Consider update the NatSpec of the functions accordingly.

2. ISablierFlow.sol#L114-L145: Consider updating the second sentence to

   If the total debt exceeds the stream balance, it returns 0.

   to match the actual behavior of the depletionTimeOf() function.

3. ISablierFlow.sol#L229: To be consistent with the NatSpec of the create() function, consider adding a comment

   A value of zero means the stream will be created with the snapshot time as block.timestamp.

4. ISablierFlow.sol#L405: Since an approved third party of the Flow NFT is allowed to set the recipient of an withdrawal to any address, consider updating the comment to

   /// - to must be the recipient if msg.sender is neither the stream's recipient nor an approved third party.

5. Errors.sol#L19-L20: Consider updating the NatSpec of the SablierFlow_InvalidTokenDecimals() error to

   Thrown when trying to create a stream with a token with decimals greater than 18.

6. Errors.sol#L67-L68: For clarity, consider updating the NatSpec of the SablierFlow_Unauthorized() error to

   /// @notice Thrown when caller lacks authorization to perform an action.

7. Helpers.sol#L8: Consider adding the following comment to the NatSpec of descaleAmount() the function:

   decimals should be less than or equal to 18.

8. Helpers.sol#L20: Consider adding the following comment to the NatSpec of scaleAmount() the function:

   decimals should be less than or equal to 18. Note: The scaled result may overflow uint256. If amount fits into uint128, the result is guaranteed not to overflow.

The following issue is identified from at commit db1d213:

1. SablierFlowBase.sol#L67-L82: The NatSepc for the notVoided() modifier is incorrect. "Not voided" does not imply "not paused", while "not paused" implies "not voided." Consider removing the 2nd sentence for notVoided(). Also, add the following comment to notPaused().

   Note that this implicitly checks that the stream is not voided either.

Recommendation

Consider implementing the above suggestions.

Sablier: Thanks for the recommendations. We’ve incorporated all of them except for the 6th one, as the current version is consistent with how we refer to the caller as msg.sender throughout ISablierFlow. The changes can be seen in PR 405, commit c28b33fc, and PR 429.

Cantina Managed: Verified.

Description

1. Errors.sol#L21-L22: The error LockupNFTDescriptor_UnknownNFT() is unused and can be removed.

Recommendation

Consider implementing the above suggestions.

Sablier: Good catch. We have removed the following error in PR 1209.

Cantina Managed: Verified.

Description

A recover() function is introduced to recover ERC-20 tokens when necessary, e.g., accidental transfers. The Lockup contract keeps track of the balance of each ERC-20 token by an aggregateBalance mapping, which is updated whenever ERC-20 tokens are transferred from or to the contract.

It should be noted that the recover() function does not support double-entry tokens, i.e., tokens that have two entry points (contracts) where users can interact with any of them to transfer tokens and call any other ERC-20 functions. In the past, SNX, sBTC, and TUSD were double-entry tokens that caused integration issues with DeFi protocols (Ref 1, Ref 2). For Lockup, if a Stream is deposited with a double-entry token from an entry point A, it would be possible to recover all the token balance using the second entry point B, which would affect the ongoing Stream.

Recommendation

Consider updating the documentation (also the Assumptions section in SECURITY.md) that a double-entry token is not supported. Generally, when recovering tokens, the protocol admin should be cautious of the side effects the token transfer would cause.

Sablier: Fixed in PR 1209. We’ve included in the Assumptions section that we only support single entry point tokens.

Cantina Managed: Verified.

Description

A recover() function is introduced to recover ERC-20 tokens when necessary, e.g., accidental transfers. The Flow contract keeps track of the balance of each ERC-20 token by an aggregateBalance mapping, which is updated whenever ERC-20 tokens are transferred from or to the contract.

It should be noted that the recover() function does not support double-entry tokens, i.e., tokens that have two entry points (contracts) where users can interact with any of them to transfer tokens and call any other ERC-20 functions. In the past, SNX, sBTC, and TUSD were double-entry tokens that caused integration issues with DeFi protocols (Ref 1, Ref 2). In this case, if a Flow is deposited with a double-entry token from an entry point A, it would be possible to recover all the token balance using the second entry point B, which would affect the ongoing Flow.

Recommendation

Consider updating the documentation (also the Assumptions section in SECURITY.md) that a double-entry token is not supported. Generally, when recovering tokens, the protocol admin should be cautious of the side effects the token transfer would cause.

Sablier: Fixed in PR 405. We’ve included in the Assumptions section that we only support single entry point tokens.

Cantina Managed: Verified.