code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/conduit/ConduitController.sol#L189-L206 https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/conduit/ConduitController.sol#L232-L257

Vulnerability details

Impact

It is well known that a malicious conduit owner can set a channel on a conduit that steals approved tokens from users. While we are making the assumption that "only the Seaport contract will be added as a channel to any conduit" for this context, I believe it is important to highlight how there are no timelock and/or restrictions on how ownership of a conduit is transferred.

Because a potentialOwner can accept ownership right after it is transferred / offerred to them, it is likely, if not certain, that users with token approvals on the conduit will not be made aware of the ownership change in time (unless the current owner notifies them independently of seaport, etc.). There are emitted events detailing the updating of the potentialOwner, and of their acceptance, but the average user is not constantly listening for events.

At minimum, the new conduit owner can close channels used by the users and disrupt their ability to use Seaport. At worst, well, we know that a malicious owner can steal funds from the existing token approvals on the conduit.

One can argue that it is the potential owner's responsibility to ensure that the new owner acts responsibly, and/or the user's job to DYOR. However, it's important to consider that the ownership can be transferred nearly instantaneously, which provides users no chance to revoke approvals.

This is also unusual in other systems like governance, etc. where there is a time period that must pass before a sensitive action like transferring ownership can be confirmed, which would give users ample time to respond.

I believe that this is a valid concern despite the objections to vulnerabilities involving malicious conduit owners from the spec, as it not very likely that a legitamite owner will transfer control to a malicious one, but it is entirely possible that their experience with and ability to trust Seaport will be negatively impacted.

Adding a timelock preventing the new potentialOwner from accepting ownership after a certain amount of time has passed would not only help prevent the aforementioned concerns, but also help safeguard against accidental calls to transferOwnership.

Proof of Concept

Alice is a user of Bob's conduit
Bob calls ConduitController:transferOwnership() to begin transferring ownership to Eve
Eve is aware that they will become the potentialOwner, so they listen for Bob's call and call ConduitController:acceptOwnership() themselves immediately after to accept ownership
Eve is now the owner of the conduit with all of its privledges
Alice is completely unaware of the ownership change until after it has happened
Anything can happen :/

Tools Used

Manual review

Recommended Mitigation Steps

Enforce that acceptOwnership() can only be called X amount of time after transferOwnership().

Ex: (very simple but you get the point)

...
uint256 _ownershipTransferredAt;
uint256 _ownershipTransferDelay;

...

function acceptOwnership(address conduit) external override {
    require(now > _ownershipTransferredAt + _ownershipTransferDelay);
    ...
}

0age commented 2 years ago

The two-step ownership transfer designed to prevent accidental ownership transfer to an account that is not properly configured, not as a measure to protect users from malicious ownership transfer (in that event, the owner in question is already behaving maliciously)

HardlyDifficult commented 2 years ago

The spirit of this report is on point but I agree with the sponsor's comment. Merging this with the warden's QA report #90

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