search

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

1 stars 0 forks source link

QA Report #212

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago
  1. ConduitController uses a two-step ownership transfer process, which is good design. However, I'd recommend renaming the initial transferOwnership() method to offerOwnership() to avoid confusion with OpenZeppelin's Ownable.transferOwnership() method, which has the same name but is an atomic transfer that requires no acceptance.

https://github.com/code-423n4/2022-05-opensea-seaport/blob/9d7ce4d08bf3c3010304a0476a785c70c0e90ae7/contracts/conduit/ConduitController.sol#L189

  1. Checks for invalid addresses are unnecessary in transferOwnership() because an invalid address cannot accept ownership. You can remove the check on lines 196-199 and remove the funciton cancelOwnershipTransfer() altogether, letting people call transferOwnership(conduit, address(0x0)) for cancelling functionality instead.

https://github.com/code-423n4/2022-05-opensea-seaport/blob/9d7ce4d08bf3c3010304a0476a785c70c0e90ae7/contracts/conduit/ConduitController.sol#L196-L199

GalloDaSballo commented 1 year ago

1 Valid NC from #207

GalloDaSballo commented 1 year ago

Also, given the context of the codebase, agree with refactoring (no need to check for address(0)) as the address(0) cannot accept ownership.

GalloDaSballo commented 1 year ago

1 R, 1NC