search

code-423n4 / 2022-05-opensea-seaport-findings

1 stars 0 forks source link

Compromised Channel Can Compromise ALL NFTs and Tokens #121

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/conduit/ConduitController.sol#L106-L179

Vulnerability details

Impact

The contest IN SCOPE section clearly states that offer items can only be transferred by a conduit that only has Seaport set as a channel. However, this condition is not true.

If a compromised channel is added to a conduit, then ALL the NFTs and tokens that the conduit has approval for are at risk/can be compromised. Till date, hacks are typically triggered by an Approval for all transaction, which limits the NFTs at risk to only one particular collection (per such transaction).

Once Seaport launches, one single hack/phishing transaction can wipe an entire wallet of all the NFTs and tokens that the conduit has access to. This is terrifying to say the least.

Proof of Concept

In the "IN SCOPE" section of this contest, one of the core invariants that Opensea expects to be upheld is:

Only items that are explicitly offered as part of a valid order may be transferred from an offerer’s account as long as they only set token approvals on either Seaport directly or on a conduit that only has Seaport set as a channel.

This means that Opensea expects all conduits created by the Seaport Conduit Controller to only have Seaport set as a channel. However, this expectation is not met or enforced by the current code.

This code indicates that the owner can add additional channels to the conduit. There is no check in the Conduit Controller that ensures a single open channel or only Seaport to be set as a channel.

In fact, the Seaport test code includes the invocation of the Conduit Controller updateChannel function here. This means that Opensea expects the owner of the conduit to be the Opensea user and so, the user can add a channel anytime (Seaport will not be an exclusive channel to the conduit).

The usual hacks that happen today are triggered by an Approval for all transaction, which limits the NFTs at risk to only one particular collection (per such transaction). With this Seaport code, one single transaction (updateChannel on the Conduit Controller) can wipe out a wallet's entire NFT collection and ERC20s (those that the conduit has approval/access to). This is VERY dangerous!

Tools Used

Visual Studio Code

Recommended Mitigation Steps

One way to solve this may be to perhaps only limit a single open channel on conduits. Additionally, on the UI Opensea can display a warning to a user who just changed the conduit channel to an address outside of Seaport

0age commented 2 years ago

This means that Opensea expects all conduits created by the Seaport Conduit Controller to only have Seaport set as a channel.

this is wholly incorrect; anyone can create a conduit and add whatever channels they want. We're fully aware that's dangerous, and so for the competition we're saying any channel other than Seaport is out of scope.

This means that Opensea expects the owner of the conduit to be the Opensea user

This is also incorrect; most users are not expected to create their own conduits (OpenSea will have a conduit, or other projects / marketplaces / power users may create their own conduits as well)

HardlyDifficult commented 2 years ago

This is a power user feature which does not introduce a vulnerability when leveraged correctly. Generally it would be a marketplace or website which would own the conduit and for this to remain secure, users must trust the conduit owner would not add a malicious channel. This is a risk which is called out in the comments as well:

Extreme care must be taken when updating channels, as malicious or vulnerable channels can transfer any ERC20, ERC721 and ERC1155 tokens where the token holder has granted the conduit approval.

The protocol is flexible to allow others to create unique offerings. This flexibility could be abused, but that potential itself is not a vulnerability.

0xleastwood commented 2 years ago

To add to this, the README explicitly outlines this as out-of-scope:

As a malicious or vulnerable conduit owner may set a channel on a conduit that allows for approved tokens to be taken at will, we make the assumption in the context of this contest that only the Seaport contract will be added as a channel to any conduit.