Platform: Code4rena
Start Date: 24/02/2022
Pot Size: $75,000 USDC
Total HM: 21
Participants: 28
Period: 7 days
Judge: alcueca
Total Solo HM: 15
Id: 94
League: ETH
Rank: 1/28
Findings: 9
Award: $18,292.76
🌟 Selected for report: 6
🚀 Solo Findings: 4
https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L325-L349 https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L299-L316
The createReserveAuction() function allows users to create duplicate auctions with the same NFT but different auctionIds. As a result, a user could back out of an active auction by creating and then cancelling a duplicate auction. This leads to locked users funds as the auction cannot be finalized on a non-existent NFT.
The following scenario outlines the exploit:
createReserveAuction() on the same NFT.cancelReserveAuction() as the auction is inactive.Manual code review.
Consider preventing users from calling createReserveAuction() if there is a pre-existing auction.
#0 - HardlyDifficult
2022-03-02T20:53:41Z
Duplicate of https://github.com/code-423n4/2022-02-foundation-findings/issues/23
This is a great find and we have put a fix in place.
https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketOffer.sol#L246-L274 https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L552-L560 https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L490-L518
NFTs can be sold to users in a variety of ways. One main method is for a seller to auction their NFT by creating a reserve auction. Once the auction is active, it is guaranteed that the sale will go to the highest bidder. Once an auction has ended, anyone can finalize the auction by calling finalizeReserveAuction(). This function will remove any auction data and transfer the NFT to auction.bidder.
Alternatively, auction.bidder can avoid redundant transfers by selling their new NFT to a user with an unexpired market offer. However, this process does not correctly transfer the NFT as seen in NFTMarketReserveAuction._transferFromEscrow(). The NFT is transferred from escrow to the auction.bidder account and not offer.buyer. As a result, auction.bidder receives the funds from the market offer while simultaneously retaining ownership of the auctioned NFT.
Consider the following scenario:
There are a couple of important conditions for this attack to be successful. The NFT being sold must be left in an unfinalized state. A user must make an offer after the auction has ended but before the auction is finalized.
Manual code review.
Consider updating NFTMarketReserveAuction._transferFromEscrow() such that the NFT is held in escrow.
By updating the statement to _finalizeReserveAuction(auctionId, true); and removing the return keyword, the highlighted should be mitigated.
#0 - HardlyDifficult
2022-03-02T17:01:55Z
Duplicate of https://github.com/code-423n4/2022-02-foundation-findings/issues/49
This is an excellent find and the report is very detailed & clear! We are implementing the recommended change.
1095.6397 USDC - $1,095.64
https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L263-L292 https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketBuyPrice.sol#L125-L141
The adminAccountMigration() function is called by the operator role to update all sellers' auctions. The auction.seller account is updated to the new address, however, the protocol fails to update buyPrice.seller. As a result, the protocol is put in a deadlock situation where the new address cannot cancel the auction and withdraw their NFT without the compromised account first cancelling the buy price and vice-versa. This is only recoverable if the new account is migrated back to the compromised account and then cancelBuyPrice() is called before migrating back.
Manual code review.
Consider invalidating the buy offer before account migration.
#0 - HardlyDifficult
2022-03-07T12:18:34Z
Correct - if we were to use adminAccountMigration while the NFT had both an auction reserve price and had a buy price set this would have created a deadlock type situation where the NFT is in a bad state and we'd likely need to migrate the NFT back to the original owner in order to correct it.
There were two possible solutions to this:
adminAccountMigration to update both auction and buy price at the same time.adminAccountMigration completely.We went with the latter solution. This is not a feature we have used in some time and as we continue to grow, it's not scalable since it required Foundation to get involved directly and included manual verification steps.
Removing this feature has saved over 2KB in contract space as well, which we really needed in order to make room for new features and changes.
🌟 Selected for report: leastwood
2434.7549 USDC - $2,434.75
The buyFromPrivateSaleFor() function allows sellers to make private sales to users. If insufficient ETH is provided to the function call, the protocol will attempt to withdraw the amount difference from the user's unlocked balance. However, if the same user has an open offer on the same NFT, then these funds will remain locked until expiration. As a result, the user cannot make use of these locked funds even though they may be needed for a successful sale.
Manual code review.
Consider adding a _cancelBuyersOffer() call to the buyFromPrivateSaleFor() function. This should be added only to the case where insufficient ETH was provided to the trade. By cancelling the buyer's offer on the same NFT, we can guarantee that the user has access to the correct amount of funds.
#0 - HardlyDifficult
2022-03-03T13:00:09Z
Yes - completely agree. This was an oversight on our end - and as a result it created an inconsistent experience for users. Since we leveraged an outstanding offer balance for a buy purchase, the same behavior should occur when using Private Sales so that user's are not in a state where they cannot make the purchase due to incorrectly having their funds locked up.
As you point out without this change, it's possible that the buyer using private sales continues to have their funds locked up for an Offer that can now only be accepted by themselves. It's an awkward state that should be avoided.
We have made the recommended change.
🌟 Selected for report: leastwood
2434.7549 USDC - $2,434.75
The _getCreatorPaymentInfo() function is utilised by _distributeFunds() whenever an NFT sale is made. The function uses try and catch statements to handle bad API endpoints. As such, a revert in this function would lead to NFTs that are locked in the contract. Some API endpoints receive an array of recipient addresses which are iterated over. If for whatever reason the function reverts inside of a try statement, the revert is actually not handled and it will not fall through to the empty catch statement.
The end result is that valid and honest NFT contracts may revert if the call runs out of gas due to an unbounded _recipients array. try statements are only able to handle external calls.
Manual code review.
Consider bounding the number of iterations to MAX_ROYALTY_RECIPIENTS_INDEX as this is already enforced by _distributeFunds(). It may be useful to identify other areas where the try statement will not handle reverts on internal calls.
#0 - HardlyDifficult
2022-03-09T11:31:05Z
Yes - this code is trying to be very defensive so that NFTs cannot get stuck in escrow. This is a great suggestion on how we could continue to improve.
Instead of capping the loop lengths in _getCreatorPaymentInfo we have opted to remove them entirely. There was another simplification that happened recently to this function that made removal a viable option. Now the only other loop for this data is in _distributeFunds and we already cap the max length there.
The primary foundation fee is charged on the initial sale of newly minted NFTs. However, the isCreator && !_nftContractToTokenIdToFirstSaleCompleted[nftContract][tokenId] check can easily be bypassed to avoid paying the primary foundation fee by selling the NFTs through a non-creator account. As a result, the creator receives a greater proportion of the sale, and the protocol generates less revenue.
A creator intends to mint and sell a new NFT. By first trading this NFT to another account, the creator can avoid ever paying the primary foundation fee and actually receive more funds for their sale.
Manual code review.
Ensure this is understood and consider updating the _getFees() function to handle this edge case properly.
#0 - HardlyDifficult
2022-03-03T12:58:05Z
🌟 Selected for report: leastwood
2434.7549 USDC - $2,434.75
https://github.com/code-423n4/2022-02-foundation/tree/main/contracts
Cryptopunks are at the core of the NFT ecosystem. As one of the first NFTs, it embodies the culture of NFT marketplaces. By not supporting the trading of cryptopunks, Foundation is at a severe disadvantage when compared to other marketplaces. Cryptopunks have their own internal marketplace which allows users to trade their NFTs to other users. As such, cryptopunks does not adhere to the ERC721 standard, it will always fail when the protocol attempts to trade them.
Here is an example implementation of what it might look like to integrate cryptopunks into the Foundation protocol.
Manual code review.
Consider designing a wrapper contract for cryptopunks to facilitate standard ERC721 transfers. The logic should be abstracted away from the user such that their user experience is not impacted.
#0 - HardlyDifficult
2022-03-03T12:45:41Z
Yes, crypto punks is an important part of the ecosystem and this is on our radar. We are not planning on adding support at this time but we will revisiting this in the future.
I like the idea of using a wrapper contract of sorts. We will be looking for a way to keep the complexity out of the market contract itself like you suggest if at all possible.
🌟 Selected for report: leastwood
2434.7549 USDC - $2,434.75
https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketOffer.sol#L255-L271 https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L557 https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L510-L515 https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketFees.sol#L188-L189
Once an auction has ended, the highest bidder now has sole rights to the underlying NFT. By finalizing the auction, fees are charged on the sale and the NFT is transferred to auction.bidder. However, if auction.bidder accepts an offer before finalization, fees will be charged on the auction.bidder sale before the original sale. As a result, it is possible to avoid paying the primary foundation fee as a creator if the NFT is sold by auction.bidder before finalization.
Consider the following scenario:
_acceptOffer() will distribute funds on the sale between Bob and Carol before distributing funds on the sale between Alice and Bob._distributeFunds() will set the _nftContractToTokenIdToFirstSaleCompleted to true, meaning that future sales will only be charged the secondary foundation fee.Manual code review.
Ensure the _nftContractToTokenIdToFirstSaleCompleted is correctly tracked. It might be useful to ensure the distribution of funds are in the order of when the trades occurred. For example, an unfinalized auction should always have its fees paid before other sales.
#0 - HardlyDifficult
2022-03-03T12:43:05Z
Yes! This is a good find.
The core of the problem is we were calling _distributeFunds before calling _transferFromEscrow where the auction finalization would be processed. We have flipped those calls so now the auction will be fully settled before we calculate fees for the offer sale.
🌟 Selected for report: leastwood
Also found by: 0x1f8b, 0xliumin, 0xwags, CertoraInc, Dravee, IllIllI, Ruhum, TerrierLover, WatchPug, cmichel, csanuragjain, defsec, gzeon, hubble, jayjonah8, kenta, kirk-baird, rfa, robee
1669.5007 USDC - $1,669.50
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.
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.
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.
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.
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.
marketLockupFor() Edge CasemarketLockupFor() will revert if too much ETH is provided. This can be modified to refund any surplus ETH to the user's FETH balance.
ERC1155 TokensMany 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.
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.
_recipients Array RestrictionThe _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.
sendValue()_buy() should make use of _sendValueWithFallbackWithdraw() instead of sendValue(). This will ensure invalid transfers are correctly handled.
#0 - HardlyDifficult
2022-03-09T11:24:29Z
Thanks for the feedback!
adminAccountMigration function completely instead. It's not something we have used in some time.increaseAllowance.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.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.#1 - alcueca
2022-03-17T09:53:27Z
Unadusted score: 100 (80 + 20 from clean formatting)
#2 - alcueca
2022-05-13T09:24:32Z
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)
#3 - liveactionllama
2022-05-17T06:44:32Z
Just a note that C4 is excluding the invalid entries from the official report.