Bid Beasts
First Flight #49
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Impact: medium
Likelihood: high
Invalid

Incorrect Ownable Initialization Allows Contract Lockout

jopantech

[M-1] Incorrect Ownable Initialization Allows Contract Lockout

Description

Both BidBeastsNFTMarketPlace.sol and BidBeasts_NFT_ERC721.sol inherit from OpenZeppelin’s Ownable, but initializes it incorrectly. In modern OpenZeppelin contracts (v5.x), the Ownable constructor does not accept parameters. Passing msg.sender to Ownable like this is invalid and may cause the contract to:

  • Fail to compile on newer OZ versions.

  • Or, if using an older fork, incorrectly assign ownership in a way not intended by the developer.

This breaks critical access control, since functions like withdrawFee() rely on onlyOwner. If ownership is not properly set, the marketplace may be permanently locked and fees cannot be withdrawn.

// @audit Incorrect Ownable Initialization.
contract BidBeastsNFTMarket is Ownable(msg.sender) { ... }
contract BidBeasts is ERC721, Ownable(msg.sender) { ... }

Risk

Likelihood:

  • High – contract will either fail at compile time or misassign ownership on deployment.

Impact:

  • Permanent lockout of fee withdrawal (platform admin cannot access accumulated fees).

  • Access control assumptions across the contract become unreliable.

Proof of Concept

/// @notice Factory that deploys a BidBeastsNFTMarket instance and can call withdrawFee()
contract MarketFactory {
address public lastDeployed;
​
function deployMarket(address _nft) external returns (address deployed) {
BidBeastsNFTMarket m = new BidBeastsNFTMarket(_nft);
deployed = address(m);
lastDeployed = deployed;
}
​
function claimFees() external {
require(lastDeployed != address(0), "no market");
BidBeastsNFTMarket(lastDeployed).withdrawFee();
}
​
// allow receiving ETH from withdrawFee()
receive() external payable {}
}
​
function test_factory_becomes_owner_and_claims_fees() public {
// Actors
address factoryDeployer = address(0xAA);
address seller = SELLER;
address buyer = BIDDER_1;
​
// Prepare balances
vm.deal(factoryDeployer, 1 ether);
vm.deal(buyer, STARTING_BALANCE);
​
// 1) Deploy Factory as factoryDeployer
vm.prank(factoryDeployer);
MarketFactory factory = new MarketFactory();
​
// 2) Use factory (still called by factoryDeployer) to deploy the real market
vm.prank(factoryDeployer);
address deployed = factory.deployMarket(address(nft));
BidBeastsNFTMarket deployedMarket = BidBeastsNFTMarket(payable(deployed));
​
// 3) Mint token to seller and list it on the deployed market
vm.prank(OWNER);
nft.mint(seller);
​
vm.startPrank(seller);
nft.approve(address(deployedMarket), TOKEN_ID);
deployedMarket.listNFT(TOKEN_ID, MIN_PRICE, BUY_NOW_PRICE); // list with buyNow
vm.stopPrank();
​
// 4) Buyer buys at buyNow to trigger _executeSale and accrue s_totalFee
vm.prank(buyer);
deployedMarket.placeBid{value: BUY_NOW_PRICE}(TOKEN_ID);
​
// Confirm fees recorded on the deployed market
uint256 fee = (BUY_NOW_PRICE * deployedMarket.S_FEE_PERCENTAGE()) / 100;
assertEq(deployedMarket.s_totalFee(), fee, "fees should be recorded");
​
// 5) The EOA who triggered factory deployment (factoryDeployer) is NOT the owner of the market.
// The factory contract should be the owner because of Ownable(msg.sender) pattern.
assertEq(deployedMarket.owner(), address(factory), "factory must be owner");
​
// 6) Attempt to withdraw fees as factoryDeployer (who deployed the factory but is not the owner) -> should revert
vm.prank(factoryDeployer);
vm.expectRevert(); // onlyOwner or not owner revert
deployedMarket.withdrawFee();
​
// 7) Now call factory.claimFees() (factory is actual owner) to withdraw fees to factory contract
// The factory `claimFees()` will call withdrawFee() on the deployed market.
uint256 beforeFactoryBalance = address(factory).balance;
vm.prank(factoryDeployer); // caller triggers factory to call withdraw (factory will be msg.sender inside withdraw)
factory.claimFees();
uint256 afterFactoryBalance = address(factory).balance;
​
// Factory should have received the fee
assertEq(afterFactoryBalance - beforeFactoryBalance, fee, "factory should have claimed the fees");
​
// And deployed market fees should be zero
assertEq(deployedMarket.s_totalFee(), 0, "market fees should be zero after withdraw");
}
forge test --match-test test_factory_becomes_owner_and_claims_fees -vvv

Recommended Mitigation

Correct ownership initialization to align with OpenZeppelin’s standards and intended control model.

- contract BidBeastsNFTMarket is Ownable(msg.sender) {
- constructor(address _nft) {
- BBERC721 = BidBeasts(_nft);
- }
- }
​
+ contract BidBeastsNFTMarket is Ownable {
+ constructor(address _nft, address _owner) {
+ BBERC721 = BidBeasts(_nft);
+ transferOwnership(_owner); // explicitly set the intended owner
+ }
+ }

The same pattern applies to BidBeasts_NFT_ERC721.sol:

- contract BidBeasts is ERC721, Ownable(msg.sender) { ... }
+ contract BidBeasts is ERC721, Ownable { ... }

Benefits of Mitigation

  • Restores correct ownership assignment.

  • Guarantees only authorized parties can execute onlyOwner functions.

  • Prevents permanent lockout of administrative functions and revenue streams.

  • Preserves marketplace and NFT contract trust.

Updates

Lead Judging Commences

cryptoghost Lead Judge 2 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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

Give us feedback!