Thread by @spearbit

“clones-with-immutable-args” is a sleek proxy pattern but…

What would a critical bug involving it look like?

@SpearbitDAO engaged @sudoswap V1 in 2022 for a security assessment and identified a critical bug in its’ contract pair validation process.

Let's break it down ⬇️

This vulnerability arises from the inadequate validation of smart contract clones in Sudoswap's "isPair()" function shown below.

The function performs the inadequate validation of contract clones via calls to the "isETH20PairClone()" and "isERC20PairClone" functions:

In "isETHPairClone()":

The `extcodecopy` function is used to get the bytecode of the queried contract. The first 54 bytes (`0x36` in hex) of the contract at the `query` address are copied to `other` which is located at `ptr + 0x40`:

The `result` is computed as the logical AND of the equality of two parts of the data. If the bytecode and the additional data stored in `ptr` match the corresponding data in `other`, `result` will be true, meaning that the queried contract is a valid clone:

What is wrong with this validation?

The vulnerability arises from the fact that the function is only checking the first `0x36` (or 54) bytes of the contract code. While this is intended to verify the 'signature' of the contract, it opens a potential attack vector.

The parameters provided in `cloneETHPair()` are directly fed into the contract.

If malicious values are supplied, these can bypass the validation process.
The cloned contract may appear legitimate but can possess a malicious payload.

Let's take a look at an example exploit ⬇️

First, we create a malicious pair by making a copy of the source of "cloneETHPair()" with malicious values for `factory`, `bondingCurve`, `nft` and `poolType` using a valid template for the connected contract and the proxy code.

However, the remaining parameters are malicious:

The attacker the initializes the new `Pair` via a copy of "initialize()" which calls "__Ownable_init()" thereby setting a malicious owner:

The malicious owner then calls the "call()" function, with the router as the target and the `calldata` for the "pairTransferERC20From()" function.

Since the checks for `onlyOwner` and `require` pass, "pairTransferERC20From()" is called with the malicious `Pair` as msg.sender:

The above function first gets the factory contract, checks if calling the target is allowed (it is because of insufficient check), and then calls the target. The router as the target contract will check if it is called from a valid pair via "isPair()":

Now, in the `pairTransferERC20From()` function, there is a check if it's being called from a valid pair by calling the `isPair()` function. The malicious pair passes this test due to the insufficient check of the first 54 bytes, allowing transfer of tokens.

Here, "isPair()" checks if the potential pair is an `enumerableETHTemplate` clone by comparing only the first 54 bytes (runtime code including implementation address).

As a result, it does not check for extra parameters `factory`, `bondingCurve`, `nft` or `poolType`:

Why is this critical?

Due to the malicious pair now being considered valid, the `require` statement in "pairTransferERC20From()" has passed and tokens can be transferred to the attacker from anyone who has set an allowance for the router.

This issue is now fixed.

Spearbit recommended Sudoswap to verify more values when checking if a pair is valid - especially the factory value and also recommend to consider the removal of all trust between pairs and routers, as well as the function "call()".

In the "isPair()" function within LSSVMPairFactory.sol the factory value is now passed into the validation functions as shown below in the following commit:

The changes in the validation code reflect an addition of the `factory` address parameter in the `isETHPairClone` and `isERC20PairClone` for functions for legitimacy and a subsequent modification in the assembly code for proper validation beyond 54 bytes of malicious parameters:

Previously, the `extcodecopy` function copied only the first 54 bytes of the contract's bytecode for comparison. Now, it copies the first 74 bytes, checking an additional 20 bytes of the bytecode to prevent an attacker from sneaking in a malicious payload.

The full security review was performed by:

- @gpersoon (LSR)
- @Mudit__Gupta (LSR)
- Max Goodman (SR)

As always, fantastic work to all involved. Keep up the great work! 🫡

This is only one of the many interesting vulnerabilities we have outlined in our report. You may view the full report located in our audits repository below:

github.com/spearbit/portf…

Interested in a security review by Spearbit?

Fill out our request form below to start working with the best security minded individuals our industry has to offer.

- Spearbit Security Review Request Form -
airtable.com/shrkxrtMKYJkLa…