Trick or Treat

The SpookySwap contract incorrectly initializes the Ownable constructor by passing msg.sender as a parameter. This misusage results in the contract owner not being set properly, leading to a critical access control vulnerability where any user can execute functions intended only for the owner.

Vulnerability Details

The SpookySwap contract inherits from OpenZeppelin's Ownable contract but incorrectly attempts to pass msg.sender to the Ownable constructor:

In OpenZeppelin's Ownable contract, the constructor does not accept any parameters. Its correct usage initializes the owner to the account deploying the contract (msg.sender) without requiring any arguments. By incorrectly passing msg.sender to the Ownable constructor, the intended owner is not set, and the owner state variable remains at its default value.

As a result, the onlyOwner modifier used throughout the SpookySwap contract does not function as intended. Functions protected by onlyOwner, such as addTreat, setTreatCost, withdrawFees, and changeOwner, become accessible to any user, compromising the contract's access control mechanisms.

Impact

• Unauthorized Access: Any user can invoke functions that are meant to be restricted to the contract owner.

• Financial Loss: Malicious users can withdraw all Ether from the contract using the withdrawFees function.

• Data Integrity Issues: Attackers can add or modify treats, altering costs and metadata, which can disrupt the contract's intended functionality.

• Ownership Takeover: An attacker can change the contract's ownership to themselves using the changeOwner function, gaining full control over all owner-restricted functions.

Proof of Concept

1. Incorrect Constructor Usage:

   The contract attempts to initialize the Ownable constructor with msg.sender:

   contract SpookySwap is ERC721URIStorage, Ownable(msg.sender), ReentrancyGuard {

   // ...

   }

   However, OpenZeppelin's Ownable contract constructor is defined without parameters:

   constructor() {

   _transferOwnership(_msgSender());

   }

2. Owner Not Set Properly:

   Due to the incorrect constructor call, the owner variable is not initialized to the deploying address. It either remains the zero address (address(0)) or is not set at all.

3. Bypassing onlyOwner Restrictions:

   Since the owner is not correctly set, the onlyOwner modifier does not restrict access. For example, any user can call the withdrawFees function:

   function withdrawFees() public onlyOwner {

   uint256 balance = address(this).balance;

   payable(owner()).transfer(balance);

   emit FeeWithdrawn(owner(), balance);

   }

4. Exploiting the Vulnerability:

   • Withdraw Funds:

     An attacker calls withdrawFees:

     await spookySwap.withdrawFees();

     This transfers all Ether in the contract to the incorrect owner address, which may be address(0) or some unintended address.

   • Change Ownership:

     An attacker can become the owner:

     await spookySwap.changeOwner(attackerAddress);

     The attacker now controls all onlyOwner functions.

   • Modify Treats:

     The attacker can add or modify treats:

     await spookySwap.addTreat("Malicious Treat", 0, "maliciousURI");

Recommendations

• Correct the Ownable Inheritance and Constructor Call:

  Remove the incorrect parameter from the Ownable inheritance:

  contract SpookySwap is ERC721URIStorage, Ownable, ReentrancyGuard {

  // ...

  }

  Initialize the Ownable contract properly in the constructor:

  constructor(Treat[] memory treats) ERC721("SpookyTreats", "SPKY") Ownable() {

  nextTokenId = 1;

  ​

  for (uint256 i = 0; i < treats.length; i++) {

  addTreat(treats[i].name, treats[i].cost, treats[i].metadataURI);

  }

  }

• Verify Owner Initialization:

  Ensure that the owner is correctly set to the deploying address (msg.sender) by default when the contract is deployed.

• Audit Access Control Modifiers:

  After correcting the inheritance, review all functions using the onlyOwner modifier to confirm they enforce access restrictions as intended.

• Test the Deployment:

  Deploy the corrected contract in a testing environment and verify that only the owner can call onlyOwner functions.

• Update Documentation:

  Reflect the changes in any associated documentation to prevent future misunderstandings regarding constructor usage.

By implementing these recommendations, the contract will correctly set the owner upon deployment, ensuring that access control mechanisms function as intended and protecting against unauthorized actions.