Smart Contract Security Audit: Sudoswap v2
Uncovering an (Almost) Out-of-Scope Mainnet Bug
Cyfrin recently conducted a comprehensive security review of Sudoswap v2 over three weeks. A total of 20 issues were found, including three high-risk and four medium-risk. This article explores the audit’s findings and the techniques used to uncover them.
Sudoswap is an NFT marketplace protocol that allows for a wide range of token types. The recent sudoAMM v2 release introduces the following new features to the platform:
- On-chain royalty support: Sudoswap’s updated version automatically enforces on-chain royalties for
ERC2981-compliant collections. The feature extends support for different royalty interfaces, like Rarible, Foundation, SuperRare, Zora, ArtBlocks, and KnownOrigin v2. - Property-checking for pools: A new feature that enables pools to set contract parameters for on-chain verification of desired properties. It opens the door to purchasing only specific items in a collection, irrespective of the verification method used.
- Revenue-sharing structure: An opt-in feature for LPs and project owners that facilitates revenue sharing. Its intention is to encourage more trading by providing a royalty incentive configured by the collection’s owner.
- ERC1155 support: This allows the creation of pools for
ERC1155<>ETHorERC1155<>ERC20pairs. - Improved operations: Separate fee accounting, unified router, enhanced event tracking, and minor gas optimizations.
For more detailed information on these features, refer to the introductory blog post and the Sudo Sundays Series by Sudoswap co-founder Owen.
Uncovering an (Almost) Out-of-Scope Mainnet Bug
The scope of an audit is often defined as a subset of contracts in a certain repository or ecosystem. For example, in-scope contracts may be defined as a set of new periphery contracts designed to interact with existing core contracts. This type of audit is often challenging for auditors without the broader context, and, therefore, time must be spent understanding the out-of-scope contracts to understand what possible attack vectors may be present for the in-scope contracts in production.
In this case, Sudoswap v2 introduced a new unifiedVeryFastRouter which allows for handling all swap types (e.g., ERC721<>ETH, ERC721<>ERC20, ERC1155<>ETH, ERC1155<>ERC20) and an efficient method for handling partial fills when buying/selling multiple items from the same pool. A version of the previous LSSVMRouter, which allows splitting swaps across multiple LSSVMPairs to purchase and sell multiple NFTs in one call, while partially in scope, was already deployed to mainnet as part of Sudoswap v1.
When reviewing the old router as part of our context-gathering efforts, we spotted a bug in the way in which slippage protection was handled in LSSVMRouter::swapNFTsForSpecificNFTsThroughETH, specifically that the user-supplied minOutput could remain locked in the router rather than being refunded. This bug was confirmed to be present in the version of LSSVMRouter deployed to mainnet, which differed only slightly in its implementation, also including another problematic function LSSVMRouter::swapNFTsForAnyNFTsThroughETH with the same bug.
The following snippet shows the code that was audited at our given commit hash:
Here, the NFT to ETH trades are performed before all else to yield an intermediate output amount. As specified by the comment, minOutputis intended to facilitate an aggregate slippage check, so outputAmountis incremented by the msg.valuesent for the ETH to NFT trades, completing the swap of NFTs for specific NFTs through ETH. Users specifying a non-zero minOutput value will have this amount deducted from the outputAmount which is sent on to the second half of the swap, from ETH to NFTs, as the inputAmountparameter of the internal function LSSVMRouter::_swapETHForSpecificNFTs.
Notice that the external function above returns the total output amount by simply adding the value of minOutput to that returned by the internal function. Given there is no transfer of funds here, it is now assumed the responsibility of this internal function to issue a refund of any unspent ETH based on this inputAmount parameter. Let’s go deeper and see what happens during the second half of the swap:
Ignoring the cached variables and declarations, the bulk of this function includes a for-loop which executes the swap, and a conditional block pertaining to whether there is any excess value to return to the sender. So, as expected, we observe refund logic within this internal function.
However, recall that inputValue does not include minOutput as this amount was deducted in the preceding function call. The actual excess value of minOutput slippage parameter is therefore not included in the remainingValue calculation and so will not be included in the subsequent ETH transfer.
If there are no intermediate underflows (due to a sufficiently large value of minOutput) then LSSVMRouter::swapNFTsForSpecificNFTsThroughETH will report an outputAmount that is not truly reflective of the user’s final ETH balance as any excess ETH as specified by minOutput will remain locked in the router forever!
Disclosure, Severity, & Remediation
Disclosure of this vulnerability was made to the Sudorandom Labs team through a channel of communication already established before the audit started. We received their initial response within a minute, concluding discussions after 45 minutes. A git diff demonstrating the issue through a Foundry fork test sent to the Sudorandom Labs team can be found in the full finding writeup.
Fortunately, following initial internal investigations and discussion with the Sudorandom Labs team, it was determined these functions have never actually been called on the mainnet deployment as they have not been connected to the Sudoswap front-end. While Sudoswap does not use these functions on the client, contract-level integrators may find themselves with potentially lost funds. Hence, the Sudorandom Labs team attempted to reach out to those potentially affected.
To summarize, this vulnerability results in the locking of user funds with high impact and likelihood. If the problematic functions were integrated into a UI, then this would be evaluated as CRITICAL, but given that the current integrations significantly reduced the likelihood, we assessed the severity as HIGH. Given that no UIs were integrated to interact with this specific function, and this router version was expected to be deprecated with the launch of Sudoswap v2, the Sudorandom Labs team acknowledged the issue.
Full Report
Our findings were documented in a comprehensive report, with relevant mitigations by the Sudoswap team.
Final Thoughts
As we conclude this review, we reiterate the pivotal role time-boxed security reviews play in smart contract security. Despite Sudoswap v2 being previously audited in a comprehensive security review, which yielded several significant security vulnerabilities, our team at Cyfrin raised many additional findings. This highlights the benefit of projects undergoing multiple audits, as each brings unique perspectives and methodologies. The Sudoswap team promptly addressed all issues, demonstrating their commitment to security and setting an exemplary standard for the industry.
The discovery of a bug in production mainnet code further underscores the value of considering the broader context while auditing smart contracts. Even while this finding was technically beyond the scope of our original audit, given LSSVMRouterwas intended to be deprecated in favor of VeryFastRouter, its discovery and resolution helped to prevent the potential for the unexpected loss of user funds in addition to being our first mainnet bug disclosure as a company.
It is important to appreciate the effectiveness of the “Swiss cheese” security model to get the best out of security engagements. Security reviews are complex, and there is no silver bullet.
For this reason, we at Cyfrin often recommend clients undergo a crowdsourced competitive audit following private time-boxed security reviews. We are incredibly excited to announce the launch of our new v0.1 smart contract audits marketplace, CodeHawks. Competitive audits are supported on launch, but we are working to make this the one-stop shop for protocols and auditors.
To learn more about smart contract security and smart contract audits, visit Cyfrin.io.
