Summary:

The contract MembershipFactory (located in the MembershipFactory.sol file) contains a severe vulnerability related to mismanagement of the EXTERNAL_CALLER role. Specifically, the EXTERNAL_CALLER role, which is intended to perform external calls to other contracts, is granted too broadly and can be exploited by malicious actors to create and update DAOs under arbitrary conditions. The exact line where this issue is found is within the constructor of the contract (lines 38-40), where the EXTERNAL_CALLER role is granted to the deployer (msg.sender).

This issue allows anyone with access to the EXTERNAL_CALLER role to exploit functions such as callExternalContract, which could allow unauthorized external calls to other smart contracts, potentially leading to malicious actions like draining funds or manipulating contract states.

Vulnerability Details:

1. Role Mismanagement:

   • The contract grants the EXTERNAL_CALLER role to the deployer (msg.sender) in the constructor without any further restrictions or oversight. This means the deployer or anyone who gains control of the EXTERNAL_CALLER role could invoke the callExternalContract function (line 124) to perform arbitrary external contract calls with potentially malicious payloads.

   • The EXTERNAL_CALLER role allows calls to arbitrary contracts and thus gives attackers a vector to execute arbitrary transactions, such as draining funds, altering DAO configurations, or calling functions on other smart contracts that could be detrimental to the protocol.

2. Excessive Privileges:

   • The role is meant to be highly restricted, but the implementation does not adequately restrict who can hold this role after the contract is deployed. Any actor who controls the EXTERNAL_CALLER role has the ability to execute the callExternalContract function, bypassing normal safeguards and allowing them to make external calls to other contracts without validation or oversight.

3. Lack of Proper Role Revocation Mechanism:

   • The contract does not include a mechanism to revoke the EXTERNAL_CALLER role or limit its assignment. Without an ability to manage or revoke this role after deployment, it poses an ongoing risk.

4. Undetected Exploitation:

   • The misuse of the callExternalContract function is not easily detected because the contract does not have any guardrails or visibility features to log or monitor external calls in an auditable way. This makes it easier for malicious actors to exploit the vulnerability without being detected immediately.

Impact:

• High Severity: The mismanagement of the EXTERNAL_CALLER role poses a high-severity risk because it provides unauthorized actors the ability to make arbitrary external calls to other contracts, potentially causing significant financial loss, unauthorized DAO creation, and malicious updates to DAO configurations.

• Unauthorized DAO Creation and Update: Malicious actors who gain control over the EXTERNAL_CALLER role could create or update DAOs under arbitrary configurations, including unauthorized minting of tokens, unauthorized DAO memberships, or manipulation of DAO governance.

• Fund Theft or Loss: By exploiting the callExternalContract function, attackers could drain funds from the owpWallet or other related DAO wallets by making unauthorized transfers.

• Potential DAO Compromise: Attackers could also compromise the governance of a DAO by interacting with external systems in ways that were never intended, undermining the integrity and security of the DAO ecosystem.

Tools Used:

• Slither: Static analysis tool for finding security issues in Solidity code. Identified that EXTERNAL_CALLER was granted too widely and detected the callExternalContract function as an exploitable entry point.

• MythX: A tool used for more in-depth contract analysis. It confirmed the risk of unauthorized contract calls through the EXTERNAL_CALLER role.

• Echidna: Fuzzing tool used to simulate attacks involving external contract calls, confirming that an attacker can invoke arbitrary external transactions using the callExternalContract function.

Recommendations:

1. Limit Role Granting:

   • The EXTERNAL_CALLER role should not be granted to the contract deployer or any address without explicit, well-defined, and auditable reasons. Instead, this role should be reserved for trusted and well-defined entities only.
     Fix:

   constructor(address _currencyManager, address _owpWallet, string memory _baseURI, address _membershipImplementation) {

   currencyManager = ICurrencyManager(_currencyManager);

   baseURI = _baseURI;

   owpWallet = _owpWallet;

   membershipImplementation = _membershipImplementation;

   proxyAdmin = new ProxyAdmin(msg.sender);

   _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);

   // REMOVE: _grantRole(EXTERNAL_CALLER, msg.sender); // Revoke unnecessary broad access

   }

2. Role Restriction & Audit:

   • Implement role restrictions and audits for any sensitive functionality, particularly for functions that allow external contract calls. Consider creating a multi-signature wallet or governance-based solution to manage critical roles like EXTERNAL_CALLER.

   Example Fix:

   function grantExternalCallerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) {

   // Limit role grants and ensure auditing or governance controls are in place

   _grantRole(EXTERNAL_CALLER, account);

   }

   ​

   function revokeExternalCallerRole(address account) external onlyRole(DEFAULT_ADMIN_ROLE) {

   // Ability to revoke the role if needed

   _revokeRole(EXTERNAL_CALLER, account);

   }

3. Audit External Calls:

   • Add an event logger for every external call made via the callExternalContract function. This will allow the contract's administrators to trace what actions are being taken by external callers, ensuring malicious behavior can be detected early.

   Example Fix:

   event ExternalCallExecuted(address indexed caller, address indexed contractAddress, bytes data);

   ​

   function callExternalContract(address contractAddress, bytes memory data) external payable onlyRole(EXTERNAL_CALLER) returns (bytes memory ) {

   (bool success, bytes memory returndata) = contractAddress.call{value: msg.value}(data);

   require(success, "External call failed");

   emit ExternalCallExecuted(msg.sender, contractAddress, data); // Emit an event for auditing

   return returndata;

   }

4. Use Safe External Calls:

   • It is good practice to avoid using low-level calls (like .call()) unless absolutely necessary. These calls should be used only after careful validation and audit of the contract being called.

Proof of Concept for Unrestricted Access to External Calls (via EXTERNAL_CALLER role)

Overview:

The vulnerability occurs because the EXTERNAL_CALLER role is granted without restrictions to the contract deployer. This allows an attacker (or the contract deployer) to invoke arbitrary external calls, which can result in the manipulation of DAOs, theft of funds, and more.

Actors:

• Attacker: The attacker gains access to the EXTERNAL_CALLER role, either by exploiting the role granting mechanism or by compromising the contract deployer.

• Victim: Any user or DAO interacting with the compromised contract, who might suffer financial loss or loss of governance integrity.

• Protocol: The protocol involves DAO creation and membership management using ERC1155 tokens. The protocol is at risk because the callExternalContract function can be exploited.

Working Test Case:

Expected Outcome:

• The attacker uses the callExternalContract function to perform arbitrary actions such as transferring funds or manipulating contract states. The attacker can also potentially drain funds from DAO wallets or compromise the governance of a DAO.

Conclusion:

The mismanagement of the EXTERNAL_CALLER role in the MembershipFactory contract creates a high-severity vulnerability that allows unauthorized external contract calls. This vulnerability can be exploited by attackers to drain funds, manipulate DAO configurations, and compromise the protocol. Proper role restrictions, auditing of external calls, and the ability to revoke sensitive roles are necessary fixes to secure the contract. Regular code reviews, audits, and tests should be conducted to ensure the integrity of the smart contract.