Summary
MembershipERC1155 proxy cannot be upgraded due to incorrect execution flow.
Vulnerability Details
When a DAO membership is created by MembershipFactory, it is deployed as a MembershipERC1155 proxy, and proxyAdmin is used.
MembershipFactory.sol#L72-L76:
TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy(
membershipImplementation,
@> address(proxyAdmin),
abi.encodeWithSignature("initialize(string,string,string,address,address)", daoConfig.ensname, "OWP", baseURI, _msgSender(), daoConfig.currency)
);
The deployed MembershipERC1155 proxy is expected to be upgraded by proxyAdmin by calling upgradeAndCall().
ProxyAdmin.sol#L38-L44:
function upgradeAndCall(
ITransparentUpgradeableProxy proxy,
address implementation,
bytes memory data
) public payable virtual onlyOwner {
proxy.upgradeToAndCall{value: msg.value}(implementation, data);
}
The expected upgrade flow would be like:
upgradeAndCall() upgradeToAndCall()
admin --------------------> proxyAdmin --------------------> proxy
Unfortunately, this simply won't work.
When a TransparentUpgradeableProxy instance is called, the call will be forwarded to _fallback() function, and only if the caller is the admin of the proxy, the proxy is upgraded.
TransparentUpgradeableProxy.sol#L95-L105:
function _fallback() internal virtual override {
@> if (msg.sender == _proxyAdmin()) {
if (msg.sig != ITransparentUpgradeableProxy.upgradeToAndCall.selector) {
revert ProxyDeniedAdminAccess();
} else {
@> _dispatchUpgradeToAndCall();
}
} else {
super._fallback();
}
}
TransparentUpgradeableProxy.sol#L88-L90:
function _proxyAdmin() internal view virtual returns (address) {
return _admin;
}
The problem is proxyAdmin is not the admin of the MembershipERC1155 proxy, as can be seen in the constructor of TransparentUpgradeableProxy, when a proxy instance is deployed, it creates an an associated ProxyAdmin instance (let's call it proxyAdmin2) to manage the proxy, so the admin of the MembershipERC1155 proxy is proxyAdmin2 instead of proxyAdmin.
TransparentUpgradeableProxy.sol#L79-L83:
constructor(address _logic, address initialOwner, bytes memory _data) payable ERC1967Proxy(_logic, _data) {
@> _admin = address(new ProxyAdmin(initialOwner));
ERC1967Utils.changeAdmin(_proxyAdmin());
}
As a result, any call to upgrade a MembershipERC1155 proxy will not be process internally but forwarded to the implementation, and the call will revert.
Therefore, to make MembershipERC1155 proxy actually be upgraded, the flow should be like:
upgradeAndCall() upgradeAndCall() upgradeToAndCall()
admin --------------------> proxyAdmin --------------------> proxyAdmin2 --------------------> proxy
However, we cannot make proxyAdmin call upgradeAndCall() in proxyAdmin2, nor can we transfer proxyAdmin2 ownership to admin.
Please follow the steps to run PoC to verify:
Follow the instructions to add foundry to the project (You may need to change hardhat version to 2.17.2 in package.json);
Create a test file (.t.sol) containing below in /test directory;
Run forge test --mt testAudit_upgradeProxy.
pragma solidity ^0.8.22;
import {Test} from "forge-std/Test.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "../contracts/dao/MembershipFactory.sol";
import "../contracts/dao/CurrencyManager.sol";
import "../contracts/dao/tokens/MembershipERC1155.sol";
import {DAOType, DAOConfig, DAOInputConfig, TierConfig} from "../contracts/dao/libraries/MembershipDAOStructs.sol";
contract Audit is Test {
address admin = makeAddr("Admin");
address owpWallet = makeAddr("owpWallet");
ERC20 WETH = new MockERC20("Wrapped ETH", "WETH", 18);
ERC20 WBTC = new MockERC20("Wrapped BTC", "WBTC", 8);
ERC20 USDC = new MockERC20("USDC", "USDC", 6);
MembershipFactory membershipFactory;
CurrencyManager currencyManager;
function setUp() public {
vm.startPrank(admin);
currencyManager = new CurrencyManager();
currencyManager.addCurrency(address(WETH));
currencyManager.addCurrency(address(WBTC));
currencyManager.addCurrency(address(USDC));
MembershipERC1155 membershipERC1155Implementation = new MembershipERC1155();
membershipFactory = new MembershipFactory(
address(currencyManager),
owpWallet,
"https://baseuri.com/",
address(membershipERC1155Implementation)
);
vm.stopPrank();
}
function testAudit_upgradeProxy() public {
address creator = makeAddr("Creator");
DAOInputConfig memory daoInputConfig = DAOInputConfig({
ensname: "SPONSORED DAO",
daoType: DAOType.SPONSORED,
currency: address(USDC),
maxMembers: 127,
noOfTiers: 7
});
vm.startPrank(creator);
address daoMemebershipProxy = membershipFactory.createNewDAOMembership(
daoInputConfig,
createTierConfigs(
daoInputConfig.noOfTiers,
ERC20(daoInputConfig.currency).decimals()
)
);
vm.stopPrank();
MembershipERC1155v2 membershipERC1155v2Implementation = new MembershipERC1155v2();
vm.startPrank(admin);
membershipFactory.updateMembershipImplementation(address(membershipERC1155v2Implementation));
ProxyAdmin proxyAdmin = membershipFactory.proxyAdmin();
vm.expectRevert();
proxyAdmin.upgradeAndCall(ITransparentUpgradeableProxy(daoMemebershipProxy), address(membershipERC1155v2Implementation), "");
vm.stopPrank();
}
function createTierConfigs(uint noOfTiers, uint8 decimals) private returns (TierConfig[] memory tiers) {
tiers = new TierConfig[](noOfTiers);
uint price = 1 * 10 ** decimals;
uint power = 1;
for (int i = int(noOfTiers) - 1; i >= 0; --i) {
uint index = uint(i);
tiers[index] = TierConfig({
amount: 2 ** index,
price: price,
power: power,
minted: 0
});
price *= 2;
power *= 2;
}
}
}
contract MockERC20 is ERC20 {
uint8 _decimals;
constructor(
string memory name_,
string memory symbol_,
uint8 decimals_
) ERC20(name_, symbol_) {
_decimals = decimals_;
}
function decimals() public view override returns (uint8) {
return _decimals;
}
}
contract MembershipERC1155v2 is MembershipERC1155 {}
Impact
MembershipERC1155 proxy cannot be upgraded.
Tools Used
Manual Review
Recommendations
There are 2 mitigations:
The simplest solution is to declare proxyAdmin as an address (EOA/Multisig) instead of an ProxyAdmin instance, then it can call proxyAdmin2 (the address can be accessed by reading the proxy's ERC-1967 admin slot) to upgrade MembershipERC1155proxy.
Or we can override TransparentUpgradeableProxy's _proxyAdmin() in a child contract, and use the child contract to create DAO membership proxy.
contract MembershipERC1155Proxy is TransparentUpgradeableProxy {
constructor(
address _logic,
address initialOwner,
bytes memory _data
) TransparentUpgradeableProxy(_logic, initialOwner, _data) {}
function _proxyAdmin() internal view override returns (address) {
return ProxyAdmin(super._proxyAdmin()).owner();
}
}
MembershipFactory.sol#L72-L76:
- TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy(
+ TransparentUpgradeableProxy proxy = new MembershipERC1155Proxy(
membershipImplementation,
address(proxyAdmin),
abi.encodeWithSignature("initialize(string,string,string,address,address)", daoConfig.ensname, "OWP", baseURI, _msgSender(), daoConfig.currency)
);
