Foundation Drop contest - 0x52's results

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L170-L219

Vulnerability details

Impact

User can bypass mint limit

Proof of Concept

if (IERC721(nftContract).balanceOf(msg.sender) + count > saleConfig.limitPerAccount) {

L183 only checks the current balance of msg.sender, not the actual number of nfts that they have minted. This allows the user to mint, send the nfts to another address then mint more.

Tools Used

Recommended Mitigation Steps

Add a mapping that stores the number of nfts a user has minted from a specific contract and update it when a user mints:

mapping(address => mapping(address => uint256)) mintCount;

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L482-L492

Vulnerability details

Impact

Other royalty recipients don't receive their share

Proof of Concept

First royalty recipient can cheat other out of their share by setting their royalty >100%. Because of the short circuit in L485 the first creator will be sent all royalties if any of the recipients are >100%.

Tools Used

Recommended Mitigation Steps

Instead of breaking the entire loop, creatorShares should be set to 0 for recipients with royalty > 10000

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L231-L271 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L279-L332

Vulnerability details

Impact

Fees do not match fees specified by contract owner

Proof of Concept

try IRoyaltyInfo(nftContract).royaltyInfo{ gas: READ_ONLY_GAS_LIMIT }(tokenId, BASIS_POINTS) returns (
    address receiver,
    uint256 /* royaltyAmount */

MarketFees.sol is designed to support any generic contract that adheres to certain interfaces. One of those interfaces is EIP-2981, however, MarketFees.sol is not fully compliant because it fails to take the proper amount of royalties. EIP-2981 specifically supports the ability for variable fees. When referencing this, MarketFees.sol disregards the amount and sends a standard fee.

Tools Used

Recommended Mitigation Steps

When using EIP-2981 compliant methods, the amount of the royalty should be passed out rather than the split.