Codehawks V2.2 Audit Contest Review
Our Codehawks audit was a wrap, and we are very excited to share the findings and the steps we took to mitigate those risks.
Written by Shubham Yadav, Protocol Engineer at Sablier Labs.
We extend our gratitude to all the auditors who participated in the contest and gave their time to find any issues in the v2.2 codebase! Importantly, no high risk issues were found.
Worth noting that the Sablier protocol, live on mainnet since 2019, has never been hacked since then, all while having tens of thousands of users since then. We take security extremely, really, seriously at Sablier Labs. Each protocol version is thoroughly audited by several parties, and our testing suite is one of the most rigorous in the industry. A testing technique called “Branching Tree Technique”, invented by Paul Razvan Berg, co-founder and CEO at Sablier Labs, even became an industry standard.
Medium Findings
M-01: Insufficient input validation allows an attacker to obtain stored XSS
The safeAssetDecimals function in the SablierV2NFTDescriptor contract wasn’t checking whether the ERC20’s symbol contains special characters. An attacker could deploy an ERC20 asset with special symbols that could be loaded on websites supporting Sablier NFT SVGs. This could be used to inject client-side scripts into web pages viewed by other users.
Resolution: We acknowledge this as a valid finding and have fixed it by supporting assets that return alphanumeric symbols and spaces only.
M-02: Overflow in the _calculateStreamedAmount function
In the case of Lockup Linear, if the start time is in the future, the streamed amount calculation can lead to arithmetic overflow due to the use of an unchecked block. This poses a serious risk to v2.2 because the cliff time could be zero if there is no cliff. In the previous versions of the protocol, streams with no cliff are created using cliff time equals to the start time. So this issue does not apply to any of the previously deployed versions.
Resolution: We have fixed this by returning 0 in _calculateStreamedAmount if the stream start time is in the future.
M-03: Limiting gas during a transaction allows skipping hook calls
The caller of withdraw, cancel, and renounce could skip hook calls by providing only enough gas to execute the Sablier functions without executing the hook callbacks. This undermines the credibility of hook calls, which are crucial for building an ecosystem around Sablier NFTs.
Resolution: We redesigned how hooks are called by adding an allowlist of contracts that are tightly integrated with withdraw and cancel and removing it from renounce. If the hook call fails, the execution fails as well. Initially, the allowlist will be maintained by the Sablier admin, with potential future governance control.
Detailed Discussion Related PR
M-04. Use of CREATE method susceptible to reorg attack
Even though reorgs are not very popular these days, we decided to mitigate this risk by switching to CREATE2 to deploy Merkle lockup contracts through the factory.
Low finding
L-01: SablierV2Lockup is not EIP-4906 compliant
Sablier protocol inherits ERC-4906 to emit MetadataUpdate events. To ensure compatibility with some NFT marketplaces, we have fixed this by supporting the ERC-4906 interface. It is worth noting that Sablier NFTs get updated on Opensea even without adherence to ERC-165.
L-02. Admin change in Merkle Lockup does not reflect in stream sender
Although a valid finding, most airstream creators use multisig wallets, so the change of admin is achieved by changing the owner of the multisig wallet. Fixing this would require a major re-architecture of the protocol since the Lockup contracts have no knowledge of the admin of the Merkle Lockup contract.
L-03 Early initiation of grace period limits creator's ability to withdraw funds via Clawback
A 7-day grace period is deemed sufficient for campaign creators to detect anomalies and use clawback to withdraw funds. It is worth noting that this 7-day period begins after the first claim. Hence, this is acknowledged as informational as we do not believe this can impact any user.
L-04. Potential honeypot attack on NFT marketplaces
This issue is more related to NFT marketplace practices than the Sablier protocol. We advise NFT marketplaces to support non-cancelable Sablier NFTs to mitigate this risk. And hence this is acknowledged as informational.
L-05. Cross chain replay attacks on Merkle Lockup contracts
While creating Airstream campaigns, it is highly unlikely for a user to use the exact same configuration across different chains. Even if they do, the Merkle root of the airdrop recipients would differ. Hence, this is acknowledged as informational as we believe this can not impact any user in practice.
L-06. Stream sender cannot cancel a stream with a pausable asset that is paused
This issue is related to ERC-20 tokens rather than the Sablier protocol. Thus, we acknowledge it as an informational finding. Mitigating this would require separating the refund from the cancel function and would impair UX by making it a two-step process. Users should exercise due diligence when accepting ERC20 tokens.
L-07. WithdrawMultiple can be DOS’ed by a random user
While this is a valid finding, we see no incentive for a user to perform a DoS attack. A user DOS’ing would pay transaction cost without gaining anything from it. Thus, this is acknowledged as informational.
Conclusion
We take security very seriously and have addressed the critical issues identified in the audit. These improvements will help us build a more robust and secure ecosystem around the Sablier protocol. We thank Codehawks for organizing this contest.
We would also like to extend our deepest thanks to all the auditors who participated in the audit contest and helped us make Sablier more secure than ever.
If you have any questions, ideas, or issues, ping us on Discord or Twitter — we’d love to hear from you.-