Wrong implementation of OperatorResolver::areOperatorsImported

This issue has been created to upgrade a QA report submission to a medium severity finding. From kenzo: Wrong implementation of OperatorResolver::areOperatorsImported The function as implemented will return true if the operators have same implementation but different selector, or different...

Schain owner dictate fund usage after kill

Lines of code Vulnerability details Impact After a schain is killed by both the owner and the IMA admin, schain admin can control all the fund using e.g. DepositBoxERC20.getFunds functions. This pose a significant centralization risk after the schain is killed. Proof of Concept function...

Check if a value is in an array before a push

This issue has been created to upgrade a QA report submission to a medium severity finding. From Dravee: Check if a value is in an array before a push In NestedRecords.sol's store function, it's possible to push an existing address token several times in the same array File: NestedRecords.sol 130...

Miners Can Manipulate tx.gasprice to Drain Users' Wallets in postIncomingMessages()

Lines of code Vulnerability details Impact The postIncomingMessages function is used to broadcast signed data indicating that a funds on an schain will be moved to mainnet. While the function performs proper signature verification, it doesn't prevent any arbitrary user from frontrunning the...

DepositBoxERC20 does not support fee-on-transfer token

Lines of code Vulnerability details Impact The transfered amount is saved without checking the actual amount of token received after the transfer. Proof of Concept saveTransferredAmountschainHash, erc20OnMainnet, amount; require ERC20Upgradeableerc20OnMainnet.transferFrom msg.sender, addressthis,...

Missing Signature Verification Leads To Critical Parameter Changes

Lines of code Vulnerability details Impact During the code review, It has been observed that, signature verification is commented out in the protocol. Without off-chain signature verification, an attacker is able to edit parameters in the protocol Proof of Concept 1. Navigate to the following...

The store method allows you to store the same token twice

This issue has been created to upgrade a QA report submission to a medium severity finding. From 0x1f8b: The store method allows you to store the same token twice in NestedRecords.solL130 To do this you must first call storenftId,tokenA,0,reserve and then storenftId,tokenA,100000,reserve because ...

An offer made after auction end can be stolen by an auction winner

Lines of code Vulnerability details Impact An Offer which is made for an NFT when auction has ended, but its winner hasn't received the NFT yet, can be stolen by this winner as transferFromEscrow being called by acceptOffer will transfer the NFT to the winner, finalising the auction, while no...

buyFromPrivateSaleFor() Will Fail if The Buyer Has Insufficient Balance Due to an Open Offer on The Same NFT

Lines of code Vulnerability details Impact 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 use...

EIP-712 signatures can be re-used in private sales

Lines of code Vulnerability details Impact Within a NFTMarketPrivateSale contract, buyers are allowed to purchase a seller's NFT. This is done through a seller providing a buyer a EIP-712 signature. The buyer can then call buyFromPrivateSaleFor providing the v, r, and s values of the signature as...

adminAccountMigration() Does Not Update buyPrice.seller

Lines of code Vulnerability details Impact 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...

Centralisation Risk: Admin Role of TokenManagerEth can Rug Pull All Eth from the Bridge

Lines of code Vulnerability details Impact There is a Centralisation risk of the bridge where the DEFAULTADMINROLE of TokenManagerEth.sol is able to modify the ERC20 token on the SChain to any arbitrary address. This would allow the admin role to change the address to one where they have infinite...

Primary seller can avoid paying the primary fee

Lines of code Vulnerability details Impact A primary seller can circumvent the 15% fee and pay 5% as a secondary seller. Context The foundation protocol charges a 15% fee if the sale is a primary sale and 5% if it's a secondary sale. There are 2 conditions that must be met for a sale to be...

[WP-H7] In some cases, when acceptOffer(), the buyer's ETH payment will be sent to the seller but the NFT will not be transferred to the buyer (paid and get nothing in return)

Lines of code Vulnerability details PoC Case A: Given: A ReserveAuction is waiting to be finalized: the creator createReserveAuction the bidder bid with 1 ETH wait until auction.endTime When: buyer makeOffer with: 1 ETH bidder acceptOffer Expected Results: 1. creator to receive bidder's payment o...

Private sale replay attack

Lines of code Vulnerability details Impact The private sale signature used in buyFromPrivateSaleFor does not prevent replay attacks. If the NFT ends up in the original seller's wallet again within the deadline it can be purchased from them again. POC S sells to B using a private sale. S creates a...

LockedBalance library should drop parameters to 96/32 bits

Lines of code Vulnerability details Impact The LockedBalance contract takes 256-bit amount values but performs bit math on them as if they were 96 bit values. Bits could spill over to a different locked balance in the else part lockedBalance stores two 128-bit locked balances in one 256-bit stora...

Private sale spoofing

Lines of code Vulnerability details Impact Similar to spoofing in finance, users can create private sales with correct signatures but then frontrun the buy with a transfer to a different wallet they control. No funds are lost as this the NFT FETH exchange is atomic but it can be bad if third...

[WP-M5] Royalties can be distribution unfairly among creatorRecipients for NFT contracts with non-standard getRoyalties() returns

Lines of code Vulnerability details Based on our research, getRoyalties is not a standardized API for NFT contracts to indicate how the royalties should be distributed among the recipients. However, in the current implementation, it always assumes that getRoyalties return in terms of BPS. if...

There is no Support For The Trading of Cryptopunks

Lines of code Vulnerability details Impact 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...

dangerous receive function

Lines of code Vulnerability details the receive function doesn't check the msg.sender is supposed to pay, risking someone to accidentally sending ether and losing it. --- The text was updated successfully, but these errors were encountered: All reactions...

[WP-M6] Inappropriate support of EIP-2981

Lines of code Vulnerability details if nftContract.supportsERC165InterfacetypeIRoyaltyInfo.interfaceId try IRoyaltyInfonftContract.royaltyInfo gas: READONLYGASLIMIT tokenId, BASISPOINTS returns address receiver, uint256 / royaltyAmount / if receiver != address0 recipients = new address payable;...

Missing receiver validation in withdrawFunds

Lines of code Vulnerability details Impact The CollateralManagement.withdrawFunds function does not validate its to parameter. Funds can be lost if to is the zero address. Similar issues have been judged as medium recently, see Sandclock M-15 / Github issue Recommended Mitigation Steps Check that...

_getCreatorPaymentInfo() is Not Equipped to Handle Reverts on an Unbounded _recipients Array

Lines of code Vulnerability details Impact 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...

Rare scenario where seller can steal NFT after an auction by creating an offer for it through reentrancy

Lines of code Vulnerability details Impact It's a very specific scenario. The seller auctions off an NFT. If the buyer relists it directly while keeping the token in the escrow, the seller has the opportunity to steal it. They do it by creating an offer for it and accepting it. Proof of Concept...

transferredAmount on mainnet can be drained if a malicious account can mint more tokens on Schain

Lines of code Vulnerability details Impact Anyone on Schain that is able to mint more tokens, other than the mint action from postMessage in tokenManagerERC20 by bridging tokens over, can potentially drain the locked tokens in transferredAmount in depositBoxERC20 on mainnet by calling exit with t...

Lack of exception handling

Lines of code Vulnerability details Impact After looking through the following functions: NFTMarketOffer.sol::264 - IERC721nftContract.transferFrommsg.sender, offer.buyer, tokenId NFTMarketPrivateSale.sol::177 - nftContract.transferFromseller, msg.sender, tokenId; We were able to reach the...

MAX_ROYALTY_RECIPIENTS_INDEX set too low

Lines of code Vulnerability details Impact The creator payouts are capped at MAXROYALTYRECIPIENTSINDEX. It's currently set to 4 and only 5 creators are paid out. Other creators are ignored. Recommended Mitigation Steps I don't think cases with more than 5 creators / royalty receivers are unlikely...

Upgradable escrow contract

Lines of code Vulnerability details Impact Upgradable escrow contract pose great risk to user who approved their NFT to the contract. Most popular token / NFT exchange do not require user approve their asset to admin upgradable contract. This also increase user gas usage because they would have t...

NFT Creators Can Avoid Paying The Primary Foundation Fee

Lines of code Vulnerability details Impact The primary foundation fee is charged on the initial sale of newly minted NFTs. However, the isCreator && !nftContractToTokenIdToFirstSaleCompletednftContracttokenId check can easily be bypassed to avoid paying the primary foundation fee by selling the...

Escrowed NFT can be stolen by anyone if no active buyPrice or auction exists for it

Lines of code Vulnerability details Impact If a NFT happens to be in escrow with neither buyPrice, nor auction being initialised for it, there is a way to obtain it for free by any actor via makeOffer, acceptOffer combination. I.e. a malicious user can track the FNDNFTMarket contract and obtain a...

NFT Sellers Can Steal Funds if an Offer is Accepted on an Unfinalized Auction

Lines of code Vulnerability details Impact 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, anyon...

Fees Are Incorrectly Charged on Unfinalized NFT Sales

Lines of code Vulnerability details Impact 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...

[WP-M3] PRIMARY_FOUNDATION_FEE can be bypassed

Lines of code Vulnerability details The protocol is charging a different fee rate when the NFT is sold by the creator for the first time on the platform, the fee rate is currently set to a constant value of 15%. For other sales, the fee rate is only 5%. if isCreator &&...

Users Can Lock Funds by Backing Out of an Auction

Lines of code Vulnerability details Impact 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 user...

Missing receiver validation in withdrawFrom

Lines of code Vulnerability details Impact The FETH.withdrawFrom function does not validate its to parameter. Funds can be lost if to is the zero address. Similar issues have been judged as medium recently, see Sandclock M-15 / Github issue Recommended Mitigation Steps Check that to != 0. --- The...

Creators can steal sale revenue from owners' sales

Lines of code Vulnerability details According to the README.md All sales in the Foundation market will pay the creator 10% royalties on secondary sales. This is not specific to NFTs minted on Foundation, it should work for any NFT. If royalty information was not defined when the NFT was originall...

NFTMarketReserveAuction: After calling adminAccountMigration to update auction.seller, other users cannot buy the NFT even before bidding starts.

Lines of code Vulnerability details Impact The adminAccountMigration function will update auction.seller to newAddress. Consider the following situation: 1. User A calls the setBuyPrice function and the createReserveAuction function to set the buy price and start the auction for his NFT. 2. The...

Gas Pricing Can Be Used To Extort Funds From Users of SChain Owner

Lines of code Vulnerability details Impact The function refundGasByUser can be exploited by the message sender to drain nodes and SChain owners of their balances when processing incoming messages. When a node collates a set of exits from an SChain to Ethereum, they are submitted on-chain via...

Exchange does not split royalty revenue correctly

Lines of code Vulnerability details According to the README.md If royalty information was not defined when the NFT was originally deployed, it may be added using the Royalty Registry which will be respected by our market contract. The actual exchange code only respects the Royalty Registry or oth...

Incorrect implementation when accepting offer from unsettled auctions

Lines of code Vulnerability details Impact FNDNFTMarket allows market offer to be made on auctioned NFTs that have not been finalised by the auction winner. Auction winner can call acceptOffer to accept the offer and finalise the auction in a single call. However, the current implementation has a...

NFT tranfered to market directly can be took by anyone

Lines of code Vulnerability details Proof of Concept 1. Offer can be made on any NFT if: 1. Not in active auction 2. No existing offer 3. The new offer amount is greater than existing offer 1. No access control on acceptOffer , anyone can send tx and reach this line which lead to transfer from...

_autoAcceptOffer doesn't check if caller of setBuyPrice owns the NFT

Lines of code Vulnerability details Impact An attacker can create an offer then auto accept that offer for an NFT they don't own. This can happen for any NFT that the contract is approved for, or any NFT left in escrow with no active limitations Proof of concept Alice has an NFT that they either...

Auction DOS On The Bidding

Lines of code Vulnerability details Impact During the code review, It has been observed that If the bidder is contract, the bidder can make auction infunctional. Proof of Concept 1. Navigate to the following contract. 2. Auction has public function named as placeBid. 3. Example Poc steps can be...

NFT owner can create multiple auctions

Lines of code Vulnerability details Impact NFT owner can permanently lock funds of bidders. Proof of concept Alice the attacker calls createReserveAuction, and creates one like normal. let this be auction id 1. Alice calls createReserveAuction again, before any user has placed a bid this is easy ...

Mapping nftContractToTokenIdToBuyPrice is not deleted correctly

Lines of code Vulnerability details Impact Users that have bought an NFT previously through the NFTMarketBuyPrice functionality will not be able to make use of this functionality again as the nftContractToTokenIdToBuyPrice mapping is not correctly deleted anywhere in the NFTMarketBuyPrice contrac...

Missing access restriction on addConnectedChain

Lines of code Vulnerability details Impact The addConnectedChain function is supposed to be called only by the CHAINCONNECTORROLE. / Requirements: - Function caller has to be granted with CHAINCONNECTORROLE. - Target chain must be different from the current. / However, there is no such restrictio...

Issues with removing a connected chain

Lines of code Vulnerability details Impact A connected chain can be removed which will make all pending messages fail. If the chain is reinitialized again at some point, its incomingMessageCounter will start at zero and allow replay attacks of all previous messages. function...

No ERC20 safe* versions called on mainnet deposit

Lines of code Vulnerability details Impact Some mainnet tokens like USDT don't correctly implement the EIP20 standard and their transfer/transferFrom function return void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert. Non-safe...

Anyone can set gas price

Lines of code Vulnerability details Impact The setGasPrice function does not have any access restrictions. Anyone can call it and set the gas price to typeuint256.max, leading to DoS in any computation that uses it. It does currently not seem to be used. Recommended Mitigation Steps Add proper...

NFT owner can change tokenURI

Lines of code Vulnerability details Impact In the ERC721OnChain implementation the token owner can set the token's URI using setTokenURI. Usually, this is token URI points to data defining the NFT attributes, images, etc.. It's usually set by the contract owner. A user that owns an NFT can just...