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

Erroneous ProxyAdmin Deployment Causing Loss of Upgrade Control

galturok

Summary

In the MembershipFactory, a vulnerability was identified in the mechanism used for managing upgradeable proxies through the ProxyAdmin pattern. The issue arises from improper chaining of ProxyAdmin contracts, which effectively disables the intended upgrade functionality of the TransparentUpgradeableProxy. This malfunction will prevent the future upgrading of proxy contracts.

Vulnerability Details

The flawed logic in the proxy administration setup stems from the following sequence of actions within the MembershipFactory contract:

  1. ProxyAdmin Initialization: The MembershipFactory constructor creates an initial ProxyAdmin contract (proxyAdminA) with msg.sender as the owner. This setup is supposed to facilitate control over proxy contracts for upgrades.

    proxyAdmin = new ProxyAdmin(msg.sender);

  2. Creation of TransparentUpgradeableProxy: When deploying a new TransparentUpgradeableProxy, proxyAdminA is used as the initialOwner input parameter:

    TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy(
    membershipImplementation,
    address(proxyAdmin),
    abi.encodeWithSignature("initialize(string,string,string,address,address)", daoConfig.ensname, "OWP", baseURI, _msgSender(), daoConfig.currency)
    );

  3. In this setup, when a TransparentUpgradeableProxy is instantiated, a new ProxyAdmin contract (proxyAdminB) is created with proxyAdminA as its owner. Here's the relevant portion from the TransparentUpgradeableProxy constructor:

constructor(address _logic, address initialOwner, bytes memory _data) payable ERC1967Proxy(_logic, _data) {
_admin = address(new ProxyAdmin(initialOwner)); // proxyAdminB
ERC1967Utils.changeAdmin(_proxyAdmin());
}

This chaining means proxyAdminA, which initially had msg.sender as its owner, relinquishes direct upgrade control when proxyAdminB becomes the actual admin of the TransparentUpgradeableProxy.

Attempts to use proxyAdminA for upgrading the proxy encounter issues because the delegated owner (msg.sender) cannot directly interact with proxyAdminB, which controls the actual proxy logic. The method upgradeAndCall is the only callable function in a ProxyAdmin contract:

function upgradeAndCall(
ITransparentUpgradeableProxy proxy,
address implementation,
bytes memory data
) public payable virtual onlyOwner {
proxy.upgradeToAndCall{value: msg.value}(implementation, data);
}

Impact

Inability to upgrade the contracts

POC

// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;
​
import "forge-std/Test.sol";
import "../contracts/dao/MembershipFactory.sol";
import "../contracts/dao/CurrencyManager.sol";
import "../contracts/dao/tokens/MembershipERC1155.sol";
import "../contracts/shared/testERC20.sol";
import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
import "@openzeppelin/contracts/access/Ownable.sol";
​
contract SetupTest is Test {
MembershipFactory public membershipFactory;
CurrencyManager public currencyManager;
MembershipERC1155 public membershipERC1155;
OWPERC20 public owpERC20;
address private owner = address(0xABCD); // Replace with some mock address
address private currencyManagerAddress = address(0x1234);
string private baseUri = "https://example.com/metadata/";
address private owpWalletAddress = address(0x9012);
​
function setUp() public {
// Deploy the CurrencyManager contract
currencyManager = new CurrencyManager();
​
// Deploy the MembershipERC1155 contract and use its address for the factory
membershipERC1155 = new MembershipERC1155();
// The actual address to be used in MembershipFactory constructor
address membershipImplementation = address(membershipERC1155);
​
// Deploy the MembershipFactory using the address of the deployed MembershipERC1155
membershipFactory = new MembershipFactory(
address(currencyManager),
owpWalletAddress,
baseUri,
membershipImplementation
);
​
// Deploy the OWPERC20 contract
owpERC20 = new OWPERC20("OWP Token", "OWP");
​
// Add the OWPERC20 token to the CurrencyManager as a whitelisted currency
currencyManager.addCurrency(address(owpERC20));
}
​
function testWrongProxyAdmin() public {
DAOInputConfig memory daoConfig = DAOInputConfig({
ensname: "testdao.eth",
daoType: DAOType.PUBLIC,
currency: address(owpERC20),
maxMembers: 100,
noOfTiers: 2
});
​
// Define TierConfig array with sample data
TierConfig[] memory tierConfigs = new TierConfig[]();
tierConfigs[0] = TierConfig({
amount: 50,
price: 1 ether,
power: 10,
minted: 0
});
tierConfigs[1] = TierConfig({
amount: 50,
price: 2 ether,
power: 20,
minted: 0
});
​
// create a new DAO membership
address newDAOMembershipAddress = membershipFactory.createNewDAOMembership(daoConfig, tierConfigs);
​
// Log the new DAO membership address
console.log("Admin Address :", address(this));
console.log("");
console.log("MembershipFactory proxyAdmin :", address(membershipFactory.proxyAdmin()));
console.log("MembershipFactory proxyAdmin Owner:", address(Ownable(address(membershipFactory.proxyAdmin())).owner()));
console.log("");
console.log("New DAO Membership Address :", newDAOMembershipAddress);
console.log("");
TransparentUpgradeableProxy proxy = TransparentUpgradeableProxy(payable(newDAOMembershipAddress));
address proxyAdminAddress = proxy._proxyAdmin();
console.log("Proxy Admin new DAO Membership :", proxyAdminAddress);
​
address proxyBOwner = Ownable(proxyAdminAddress).owner();
console.log("Proxy Admin new DAO Owner :", proxyBOwner);
}
}
​

LOGS

Logs:
Admin Address : 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
​
MembershipFactory proxyAdmin : 0x4f81992FCe2E1846dD528eC0102e6eE1f61ed3e2
MembershipFactory proxyAdmin Owner: 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
​
New DAO Membership Address : 0xCB6f5076b5bbae81D7643BfBf57897E8E3FB1db9
​
Proxy Admin new DAO Membership : 0x3F919fAa6262122aeE2Db9EA8450321E5D5F2532
Proxy Admin new DAO Owner : 0x4f81992FCe2E1846dD528eC0102e6eE1f61ed3e2
​

Explanation of Logs

  1. Admin Address:

    • The Admin Address 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496 represents the intended admin, likely the deployer's address, or another address designated to control the deployment.

  2. MembershipFactory ProxyAdmin:

    • The MembershipFactory proxyAdmin is logged as 0x4f81992FCe2E1846dD528eC0102e6eE1f61ed3e2.

    • This address corresponds to proxyAdminA, which was initially created with msg.sender as its owner, confirmed by the MembershipFactory proxyAdmin Owner being 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496.

  3. New DAO Membership and Proxy Chain:

    • The New DAO Membership Address is 0xCB6f5076b5bbae81D7643BfBf57897E8E3FB1db9, representing the deployed TransparentUpgradeableProxy.

    • The Proxy Admin new DAO Membership is logged as 0x3F919fAa6262122aeE2Db9EA8450321E5D5F2532 and is equivalent to proxyAdminB, which is instantiated within the proxy and governs this new membership proxy contract.

  4. Chained Ownership Breakdown:

    • The Proxy Admin new DAO Owner is 0x4f81992FCe2E1846dD528eC0102e6eE1f61ed3e2, indicating that proxyAdminA owns proxyAdminB.

    • This owner relationship highlights the problem where the Admin Address (0x7FA9385...) intends to control upgrades but is indirectly routed through a proxy chain (proxyAdminB controlling the proxy, owned by proxyAdminA).

Tools Used

Foundry

Recommendations

  • Instead of creating a new ProxyAdmin and passing this as the initial owner to TransparentUpgradeableProxy, the contract should directly utilize the address of the intended external administrator like the admin address.

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.