NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View all submissions
Previous Next
Submission Details
Impact: low
Likelihood: low

Inconsistent Access Control Pattern — `cancelListing` Uses Inline `require` Instead of `onlySeller` Modifier

Author Revealed upon completion

Links

  • src/NFTDealers.sol:161-164 — cancelListing() with inline seller check

  • src/NFTDealers.sol:76-79 — onlySeller modifier definition

  • src/NFTDealers.sol:175 — collectUsdcFromSelling() using onlySeller modifier

  • src/NFTDealers.sol:189 — updatePrice() using onlySeller modifier

Vulnerability Details

The cancelListing function performs the seller authorization check via an inline require statement:

// NFTDealers.sol:164
require(listing.seller == msg.sender, "Only seller can cancel listing");

Meanwhile, collectUsdcFromSelling and updatePrice use the dedicated onlySeller modifier for the same check:

// NFTDealers.sol:76-79
modifier onlySeller(uint256 _listingId) {
require(s_listings[_listingId].seller == msg.sender, "Only seller can call this function");
_;
}

Both approaches are functionally equivalent, but the inconsistency means that if the seller authorization logic ever needs to change, cancelListing would need to be updated separately. The modifier exists specifically to centralize this check.

Impact

No direct security impact — the protection is functionally identical. However, the inconsistency reduces code maintainability and increases the risk of divergent behavior if the seller check logic is updated in the modifier but not in the inline require.

Recommended Mitigation

Replace the inline require with the onlySeller modifier:

- function cancelListing(uint256 _listingId) external {
- Listing memory listing = s_listings[_listingId];
- if (!listing.isActive) revert ListingNotActive(_listingId);
- require(listing.seller == msg.sender, "Only seller can cancel listing");
+ function cancelListing(uint256 _listingId) external onlySeller(_listingId) {
+ Listing memory listing = s_listings[_listingId];
+ if (!listing.isActive) revert ListingNotActive(_listingId);

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!