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

Wrong access control.

timenov

Summary

The MembershipFactory is inheriting from AccessControl and has 2 roles: DEFAULT_ADMIN_ROLE and EXTERNAL_CALLER. However the EXTERNAL_CALLER role is not used correctly.

Vulnerability Details

The DEFAULT_ADMIN_ROLE is used for granting/revoking roles, and calling the following functions: setCurrencyManager, setBaseURI and updateMembershipImplementation. The EXTERNAL_CALLER is supposed to call only callExternalContract. The problem is that updateDAOMembership function has onlyRole(EXTERNAL_CALLER) modifier. This is admin function and is not supposed to be called by other roles.

function updateDAOMembership(string calldata ensName, TierConfig[] memory tierConfigs)
external onlyRole(EXTERNAL_CALLER) returns (address) {
address daoAddress = getENSAddress[ensName];
require(tierConfigs.length <= TIER_MAX, "Invalid tier count.");
require(tierConfigs.length > 0, "Invalid tier count.");
require(daoAddress != address(0), "DAO does not exist.");
DAOConfig storage dao = daos[daoAddress];
if(dao.daoType == DAOType.SPONSORED){
require(tierConfigs.length == TIER_MAX, "Invalid tier count.");
}
​
uint256 maxMembers = 0;
​
// Preserve minted values and adjust the length of dao.tiers
for (uint256 i = 0; i < tierConfigs.length; i++) {
if (i < dao.tiers.length) {
tierConfigs[i].minted = dao.tiers[i].minted;
}
}
​
// Reset and update the tiers array
delete dao.tiers;
for (uint256 i = 0; i < tierConfigs.length; i++) {
dao.tiers.push(tierConfigs[i]);
maxMembers += tierConfigs[i].amount;
}
​
// updating the ceiling limit acc to new data
if(maxMembers > dao.maxMembers){
dao.maxMembers = maxMembers;
}
​
dao.noOfTiers = tierConfigs.length;
return daoAddress;
}

https://github.com/Cyfrin/2024-11-one-world/blob/main/contracts/dao/MembershipFactory.sol#L100-L134

Impact

Medium, because initially both roles are given to msg.sender, but can change in future and also this is giving access to unrestricted address to call admin function.

Tools Used

Manual Review

Recommendations

Change the modifier from onlyRole(EXTERNAL_CALLER) to onlyRole(DEFAULT_ADMIN_ROLE). By making this change the EXTERNAL_CALLER can still call this function by calling callExternalContract().

Updates

Lead Judging Commences

0xbrivan2 Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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