QA Report

[code423n4]

QA Report

Overall I would recommend rethinking the architecture here. Some of the abstract contracts are leaky abstractions and have many cross dependencies.

Just an idea, have one contract that serves as a vault for the NFTs and manages all information about escrow for buy price / auction. That way there isn't so much state that you have to manage between all the different contracts and there's one contract that manages whether a given NFT is in escrow.

Low

Use safeTransferFrom / safeTransfer for ERC721

It's best practice to use this functions so NFTs don't get transferred to contracts that aren't aware of the protocol. Make sure you account for reentrancy though.

Large unchecked blocks

All the large unchecked blocks can potentially cause unexpected overflow issues in arithmetic. Make sure very unchecked block targets a very specific block of code (as opposed to large for loop blocks).

Non-critical

Incorrect comments in role mixins

These should be revokeRole, not grantRole
https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/mixins/roles/AdminRole.sol#L35

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/mixins/roles/OperatorRole.sol#L26

[HardlyDifficult]

Thanks for the feedback!

• Architecture: Another user suggested something very similar. It is very tempting to go that route, it does seem like it would clean up the code and offer a clear separation of responsibilities. However it may prevent us for using efficient slot packing and would be a very significant change to make shortly before our launch.
• Use safeTransferFrom: This is a common recommendation but it does increase exposure to potential reentrancy issues. We do use nonReentrant for many calls, but we'd still like to limit external calls where possible just in case the modifier was missed in one of our flows. I'm not currently aware of a good use case which would require this -- but it is certainly something we are considering adding in the future.
• Large unchecked blocks: We have comments around the usage of unchecked throughout the code. If there are any which are unsafe or not documented, we should look into those closer. In general, the reason for the large unchecked blocks is usually due to limitations with Solidity. For instance, in order to uncheck the ++i portion of a for loop, we need to wrap the entire loop as unchecked since Solidity does not yet have a way of scoping the unchecked section in-line. Once that capability is added, we will clean up the code to make unchecked blocks more targeted and easier to reason about.
• Incorrect comments: This file is listed as out of scope -- however you are correct! This was a copy paste error in the comments and we have corrected them.

[alcueca]

Unadjusted score: 55 (inclidung 30 points for the extensive architecture recommendations).

[alcueca]

Extra 5 points from #24

[alcueca]

Extra 5 points from #22.