QA Report

[code423n4]

Signature Re-Use

The adminAccountMigration() function intends to allow a seller to update auction.seller in the event their account is compromised. However, the signature in requireAuthorizedAccountMigration() can be re-used because there is no use of a nonce to prevent this.

Consider adding a nonce to account for this behaviour.

Approval Race Condition

There is no use of the increaseAllowance() and decreaseAllowance() functions. As a result, users could frontrun calls to approve() by using the allocated amount before the new amount is set. Consequently, it might be possible for the approved spender to spend more tokens then they were allocated.

Ensure this is understand and consider adding the aforementioned functions.

Unclear Function Naming

The _cancelBuyersOffer() function isn't exactly clear as it will only cancel the offer if the buyer is msg.sender. Therefore, it would be more clear to rename this to _cancelSellersOffer() or similar to avoid confusion.

Improper BASIS_POINTS Check in _distributeFunds()

The _distributeFunds() function checks if creatorShares[i] > BASIS_POINTS when it should actually be checking if totalShares > BASIS_POINTS. This ensures that the sum of creator shares do not exceed an expected amount.

Improper State Handling

If _autoAcceptBuyPrice() is executed by a new buyer that is not offer.buyer, then there will be an excess of ETH. The original offer.buyer will then have to wait until the offer expires as it isn't invalidated.

Ensure this is understood.

Unhandled marketLockupFor() Edge Case

marketLockupFor() will revert if too much ETH is provided. This can be modified to refund any surplus ETH to the user's FETH balance.

No Support For ERC1155 Tokens

Many new NFT contracts adhere to the ERC1155 standard. Currently, it is not possible to trade these tokens on the Foundation marketplace.

Consider making the necessary integrations.

Malicious Contract Upgrades Will Lock Funds And NFTs

If the protocol owner becomes malicious or their account is compromised for whatever reason, they can lock users' funds by upgrading the relevant contracts to an malicious contract which always reverts. As a result, NFTs and ETH will be forever locked by the protocol.

Unclear _recipients Array Restriction

The _recipients array is restricted to just 5 receivers. Ensure this is understand by NFT creators as they may intend to use additional receivers but these receivers may be disproportionally rewarded if the majority of creator shares are not included.

Inconsistent Use of sendValue()

_buy() should make use of _sendValueWithFallbackWithdraw() instead of sendValue(). This will ensure invalid transfers are correctly handled.

[HardlyDifficult]

Thanks for the feedback!

• Signature re-use: In this use case, signature reuse is actually desirable. However after a different issue was reported we opted to remove the adminAccountMigration function completely instead. It's not something we have used in some time.
• Approval race condition: Yes, we will address this. ATM it's not a concern because approvals won't be used on Foundation thanks to the special market relationship, but we'll want to change this in the future. It does expose a potential vulnerability related to another contract, not included in this review, so we are looking to patch that before adding increaseAllowance.
• Unclear naming: Yes, we agree. The function has been renamed.
• Improper BASIS_POINTS Check: This is a fair point. It's somewhat an arbitrary decision though. We are expecting values to be in BP and this logic is simply trying to handle invalid results gracefully. Either way we bring the total back down to 10% and guard against overflowing. ATM we are going to leave it as is.
• Improper State Handling: This is a valid concern but currently working as designed. When auto buy now applies we treat this the same as if they called buy directly. When someone purchases using buy we have opted to allow the highest offer to remain valid. It may not be common, but it's not 100% clear that the offer should have been canceled. e.g. the new owner may accept that offer in order to have a fast exit at a small loss due to buyers remorse.
• Unhandled marketLockupFor() Edge Case: ATM there is no use case we are aware of where it would make sense for too much ETH to be provided. Because of this we are going to revert instead of refunding - it'll save a bit of gas for other users and may prevent an invalid transaction (possibly user error or a frontend bug).
• No Support For ERC1155 Tokens: Agree, we will add support for ERC-1155 at some point. It's a non trivial change though and we don't want to rush into it just yet.
• Malicious Contract Upgrades: Agree. Obviously we have no desire to do something like this. Longer term we are looking to progressively decentralized - possibly by giving upgrade permissions to a DOA or even removing that capability completely. For now though, this is a fast and easy way to continue adding features to our market.
• Unclear _recipients Array Restriction: Agree. We need some cap in order to limit amount of gas that non-creators would be required to pay. 5 is arbitrary and we should at least provide clear documentation. For now, we have added some comments about this inline. Longer term we can continue to communicate this more clearly, and encourage those with more than 5 recipients to use solutions such as https://www.0xsplits.xyz/.
• Inconsistent Use of sendValue(): Yes, this is inconsistent. However the use case here is slightly different. Generally the use of sendValueWithFallbackWithdraw is when we are sending ETH to an address other than the msg.sender - we want to ensure that a malicious user cannot cause other user's transactions to revert. Here the refund is going to the msg.sender - if they are non-receivable they should be able to retry using the correct value instead. So we are leaving it as-is for now.

[alcueca]

Unadusted score: 100 (80 + 20 from clean formatting)

[alcueca]

Signature Re-Use - Non-Critical (Lack of documentation on desiderability) Approval Race Condition - Invalid in my opinion, since this has never been exploited and is uneconomical to exploit. Unclear Function Naming - Non-Critical Improper BASIS_POINTS Check in _distributeFunds() - Non-Critical (Lack of documentation on trade-offs) Improper State Handling - Non-Critical (Lack of documentation on feature set) Unhandled marketLockupFor() Edge Case - Low No Support For ERC1155 Tokens - Non-Critical (Accepted suggestion) Malicious Contract Upgrades Will Lock Funds And NFTs - Invalid, this kind of issue belongs in a governance audit. Unclear _recipients Array Restriction - Non-Critical (Lack of documentation on trade-offs) Inconsistent Use of sendValue() - Non-Critical (Lack of documentation on flow and trade-offs)

[liveactionllama]

Just a note that C4 is excluding the invalid entries from the official report.