Project

One World
NFTDeFi
15,000 USDC
View results
Submission Details
Severity: high
Valid

Upgradability Locked Due to ProxyAdmin Misconfiguration

Summary

The upgradability of the TransparentUpgradeableProxy is effectively bricked due to the unexpected inputs provided to its constructor. This creates a critical vulnerability, rendering the contract's upgradeability non-functional. The issue arises because the constructor deploys a new ProxyAdmin instance, while an existing ProxyAdmin instance is also passed as an input. This results in a conflict where the ownership and administrative control mechanisms are misaligned, ultimately making it impossible to upgrade the contract's implementation.

Vulnerability Details

The function createNewDAOMembership creates a new DAO membership by deploying a TransparentUpgradeableProxy. The TransparentUpgradeableProxy constructor accepts three parameters: address _logic, address initialOwner, and bytes memory _data. The constructor implementation is as follows:

constructor(address _logic, address initialOwner, bytes memory _data) payable ERC1967Proxy(_logic, _data) {
_admin = address(new ProxyAdmin(initialOwner));
// Set the storage value and emit an event for ERC-1967 compatibility
ERC1967Utils.changeAdmin(_proxyAdmin());
}

As shown in the code, the TransparentUpgradeableProxy constructor deploys a new instance of the ProxyAdmin contract, with the specified initialOwner as its owner.

The createNewDAOMembership function, however, contains the following logic:

TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy(
membershipImplementation,
address(proxyAdmin),
abi.encodeWithSignature(
"initialize(string,string,string,address,address)",
daoConfig.ensname,
"OWP",
baseURI,
_msgSender(),
daoConfig.currency
)
);
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);
_grantRole(EXTERNAL_CALLER, msg.sender);
}

Here, the createNewDAOMembership function passes an existing ProxyAdmin instance (proxyAdmin) to the constructor of TransparentUpgradeableProxy. However, the TransparentUpgradeableProxy constructor also deploys its own ProxyAdmin instance. As a result, the following issues arise:

  1. The admin of the TransparentUpgradeableProxy is a newly deployed ProxyAdmin instance.

  2. The owner of this new ProxyAdmin instance is another ProxyAdmin instance, as the owner of the ProxyAdmin contract is set during its construction.

Consequently:

  • The deployer of the MembershipFactory contract indirectly owns the ProxyAdmin controlling the proxy, as they own the initial ProxyAdmin instance.

  • Since the ownership of the ProxyAdmin contract is itself assigned to another ProxyAdmin, there is no direct mechanism to update the ownership of the controlling ProxyAdmin.

  • This design flaw creates a situation where the TransparentUpgradeableProxy's implementation contract cannot be upgraded, as there is no accessible mechanism to change the admin or update the logic.

This issue highlights a critical gap in the contract design, as the ownership hierarchy inadvertently renders the upgradeability of the proxy non-functional. To resolve this, the contract design must be adjusted to avoid redundant or conflicting ownership assignments.

Impact

Upgrade to a new implementation is not possible

Tools Used

Manual review

Recommendations

Input initialOwner instead of instance of ProxyAdmin in createNewDAOMembership

Updates

Lead Judging Commences

0xbrivan2 Lead Judge
10 months ago
0xbrivan2 Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

MembershipERC1155 implementations can not be upgraded for already deployed proxies

Support

FAQs

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