search

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

1 stars 0 forks source link

QA Report #73

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

1) Use Whitelist to Handle Malicious Actors

Risk Level: Informational

Impact

The known limitation have full list of how Malicious Actors can abuse this project to cheat users money.

Proof of Concept

https://github.com/code-423n4/2022-05-opensea-seaport

Recommended Mitigation Steps

The Seaport Operator or DAO should able to Whitelist those ERC20/721/1155 and conduit/channel. This will effectively block most if not all Malicious Actors.

2) Consider Add Renounce Conduit Ownership Function in ConduitController.sol

Risk Level: Informational

Impact

In ConduitController.sol, there is transferOwnership() with zero address check which is good.

But different projects/people may have different thinking, some of them may want to Renounce their Conduit Ownership in future without passing it to next person.

Proof of Concept

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/ConduitController.sol

Recommended Mitigation Steps

Consider to Add Renounce Conduit Ownership Function in ConduitController.sol

3) Unclear Function Used by Who or Which Role

Risk Level: Informational

Impact

There are a lot of sol files. When read the contract code, always wondering these function used by who. For example:

@notice OrderCombiner contains logic for fulfilling combinations of orders,
 *         either by matching offer items to consideration items or by
 *         fulfilling orders where available.
* @notice Internal function to attempt to fill a group of orders, fully or
*         partially, with an arbitrary number of items for offer and
*         consideration per order alongside criteria resolvers containing
*         specific token identifiers and associated proofs.

The fulfillers, Conduit, Channel or random person?

Proof of Concept

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/OrderCombiner.sol More...

Recommended Mitigation Steps

Suggest to make it clear Who/Which Role to call these functions.

0age commented 2 years ago

low signal, ownership can be renounced by sending to a stub owner

HardlyDifficult commented 2 years ago

Consider Add Renounce Conduit Ownership Function in ConduitController.sol

This seems to be a reasonable consideration. Although a stub could be used, with the 2-step transfer process it requires a contract to do and is therefore harder for 3rd parties to confirm it's been revoked.

The other comments don't seem valid.

GalloDaSballo commented 2 years ago

Agree with Judge