diff --git a/solidity/README.md b/solidity/README.md index e01b1539..fadc9793 100644 --- a/solidity/README.md +++ b/solidity/README.md @@ -168,8 +168,9 @@ npx hardhat upgrade --network sapphire-testnet --address 0xad3C76e4E621C0cfF7540 npx hardhat upgrade --network sapphire --address ``` -*Option 2:* With `SECRET_KEY` used to deploy an implementation only; the -owner is Safe account that will be signed and submitted afterwards: +*Option 2:* With `SECRET_KEY` used to deploy an implementation only. The +owner is Safe account that will sign and execute upgrade transactions +separately: ```shell # Sapphire Testnet @@ -179,6 +180,15 @@ npx hardhat upgrade --network sapphire-testnet --address 0xad3C76e4E621C0cfF7540 npx hardhat upgrade --network sapphire --address --output-safe accounting-upgrade-safe.json ``` +This will generate two Safe Transaction Builder JSON files: + +1. the `proposeUpgrade()` transaction with suffix -1 +2. the `upgradeToAndCall()` transaction with suffix -2 + +Both transactions need to be separately signed and submitted in order to upgrade +the Accounting contracts. Combining them inside a single batch is not possible +due to the simulation attack gated by `UPUPSUpgradeable`. + ### Check status You can view all public info of the contract by running: diff --git a/solidity/contracts/Accounting.sol b/solidity/contracts/Accounting.sol index 1a420998..8e3b9fe6 100644 --- a/solidity/contracts/Accounting.sol +++ b/solidity/contracts/Accounting.sol @@ -15,7 +15,7 @@ import { } from "./Types.sol"; import {IAccountingSiweAuth} from "./interfaces/IAccountingSiweAuth.sol"; import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; -import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; +import {UPUPSUpgradeable} from "./lib/UPUPSUpgradeable.sol"; /** * @title Accounting @@ -25,7 +25,7 @@ import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/U * addresses derived on-chain from contract's secretKey. Fund locking, P2P transfers, * and automated withdrawals via EIP-712 signatures. */ -contract Accounting is EIP712SignatureVerifier, EVMSignerAndVerifier, OwnableUpgradeable, UUPSUpgradeable { +contract Accounting is EIP712SignatureVerifier, EVMSignerAndVerifier, OwnableUpgradeable, UPUPSUpgradeable { /// @notice Contract version, bumped on each upgrade for tracking/verification. uint64 public constant VERSION = 1; @@ -142,12 +142,18 @@ contract Accounting is EIP712SignatureVerifier, EVMSignerAndVerifier, OwnableUpg __Accounting_init(_roflAppID, _owner); } + /** + * @notice Authorizes the upgrade proposal. + * @dev Required by UPUPSUpgradeable. Only the contract owner can propose the upgrade. + */ + function _authorizeProposeUpgrade() internal override onlyOwner {} + /** * @notice Authorizes an upgrade to a new implementation. * @dev Required by UUPSUpgradeable. Only the contract owner can upgrade. * @param newImplementation Address of the new implementation contract */ - function _authorizeUpgrade(address newImplementation) internal override onlyOwner {} + function _authorizeUpgrade(address newImplementation) internal override onlyOwner acceptProposedUpgrade(newImplementation) {} /// @dev Ownership renunciation is disabled to prevent bricking the proxy. function renounceOwnership() public pure override { diff --git a/solidity/contracts/lib/UPUPSUpgradeable.sol b/solidity/contracts/lib/UPUPSUpgradeable.sol new file mode 100644 index 00000000..338dba7d --- /dev/null +++ b/solidity/contracts/lib/UPUPSUpgradeable.sol @@ -0,0 +1,176 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.0; + +import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; + +/** + * @title Universal Proposeable Upgradeable Proxy Standard (UPUPS) + * @notice Extends UUPSUpgradeable with a two-step upgrade process that prevents + * silent simulated upgrades that could extract confidential contract state. + * The upgrader must first call `proposeUpgrade` to announce the new + * implementation, and then call regular `upgradeToAndCall` in a later block. + * + * #### Example + * + * ```solidity + * contract MyContract is UPUPSUpgradeable, OwnableUpgradeable { + * function initialize(address _owner) public initializer { + * __Ownable_init(_owner); + * } + * + * // Gate proposeUpgrade with appropriate modifier. + * function _authorizeProposeUpgrade() internal onlyOwner { } + * + * // Gate UUPSUpgradeable.authorizeUpgrade with additional acceptProposedUpgrade() modifier. + * function _authorizeUpgrade(address newImpl) internal onlyOwner acceptProposedUpgrade(newImpl) { } + * } + * ``` + */ +abstract contract UPUPSUpgradeable is UUPSUpgradeable { + /// @custom:storage-location erc7201:oasisprotocol.storage.UPUPSUpgradeable + struct UPUPSUpgradeableStorage { + address _newImplementation; + bytes32 _newImplementationHash; + uint256 _minBlockNumber; + } + + // keccak256(abi.encode(uint256(keccak256("oasisprotocol.storage.UPUPSUpgradeable")) - 1)) & ~bytes32(uint256(0xff)) + bytes32 private constant UPUPSUpgradeableStorageLocation = + 0x2ba9668233a4827367587178d890a758d4583dfd9e5c9ef8b58defb278a6ad00; + + function _getUPUPSUpgradeableStorage() + private + pure + returns (UPUPSUpgradeableStorage storage $) + { + assembly { + $.slot := UPUPSUpgradeableStorageLocation + } + } + + error ImplementationDoesNotMatch(); + error ImplementationHashDoesNotMatch(); + error MinBlockNumberNotReached(); + error MinBlockNumberInPast(); + error NewImplementationNotAContract(); + + event UpgradeProposed( + address indexed newImplementation, + bytes32 indexed newImplementationHash, + uint256 indexed minBlockNumber + ); + + event UpgradeAccepted( + address indexed newImplementation, + bytes32 indexed newImplementationHash, + uint256 indexed minBlockNumber + ); + + /** + * @notice Initializes the contract. + */ + function __UPUPSUpgradeable_init() + internal + onlyInitializing + { + } + + /** + * @notice Checks if the proposed upgrade is valid and clears the proposed + * upgrade bits. + */ + modifier acceptProposedUpgrade(address newImplementation) { + _acceptProposeUpgrade(newImplementation); + + _; + } + + /** + * @notice Function that should revert when `msg.sender` is not authorized + * to propose the upgrade. Use {proposedUpgradeImplementation} and + * {proposedUpgradeMinBlockNumber} to fetch current proposal details. Called + * by {proposeUpgrade}. + * + * Normally, this function will use an xref:access.adoc[access control] + * modifier such as {Ownable-onlyOwner}. + * + * ```solidity + * function _authorizeProposeUpgrade() internal onlyOwner {} + * ``` + */ + function _authorizeProposeUpgrade() internal virtual; + + /** + * @notice Returns the new implementation address of the proposed upgrade. + */ + function proposedUpgradeImplementation() public view virtual returns (address) { + UPUPSUpgradeableStorage storage $ = _getUPUPSUpgradeableStorage(); + return $._newImplementation; + } + + /** + * @notice Returns the hash of the implementation runtime code of the proposed upgrade. + */ + function proposedUpgradeImplementationHash() public view virtual returns (bytes32) { + UPUPSUpgradeableStorage storage $ = _getUPUPSUpgradeableStorage(); + return $._newImplementationHash; + } + + /** + * @notice Returns the proposed upgrade minimum block number. + */ + function proposedUpgradeMinBlockNumber() public view virtual returns (uint256) { + UPUPSUpgradeableStorage storage $ = _getUPUPSUpgradeableStorage(); + return $._minBlockNumber; + } + + /** + * @notice Reverts unless the implementations matches and the current block + * number is at least the minBlockNumber. + */ + function _acceptProposeUpgrade(address newImplementation) internal virtual { + if (newImplementation != proposedUpgradeImplementation()) { + revert ImplementationDoesNotMatch(); + } + if (newImplementation.codehash != proposedUpgradeImplementationHash()) { + revert ImplementationHashDoesNotMatch(); + } + if (block.number < proposedUpgradeMinBlockNumber()) { + revert MinBlockNumberNotReached(); + } + emit UpgradeAccepted(newImplementation, newImplementation.codehash, proposedUpgradeMinBlockNumber()); + + UPUPSUpgradeableStorage storage $ = _getUPUPSUpgradeableStorage(); + $._newImplementation = address(0); + $._minBlockNumber = 0; + } + + /** + * @notice Propose the upgrade to the new implementation address after the + * given block number. If minBlockNumber is zero, take the number of the + * next block. + * @dev minBlockNumber is intentionally left to the caller's discretion: + * the function guards against the simulated upgrade attack, but a longer + * window may be provided for the new contract implementation review. + */ + function proposeUpgrade(address newImplementation, uint256 minBlockNumber) public virtual { + if (newImplementation.code.length == 0) { + revert NewImplementationNotAContract(); + } + if (minBlockNumber == 0) { + minBlockNumber = block.number+1; + } + if (minBlockNumber <= block.number) { + revert MinBlockNumberInPast(); + } + + UPUPSUpgradeableStorage storage $ = _getUPUPSUpgradeableStorage(); + $._newImplementation = newImplementation; + $._newImplementationHash = newImplementation.codehash; + $._minBlockNumber = minBlockNumber; + + _authorizeProposeUpgrade(); + + emit UpgradeProposed(newImplementation, newImplementation.codehash, minBlockNumber); + } +} diff --git a/solidity/tasks/deploy.ts b/solidity/tasks/deploy.ts index 492a14c4..977b61e9 100644 --- a/solidity/tasks/deploy.ts +++ b/solidity/tasks/deploy.ts @@ -4,7 +4,7 @@ import { join } from "path"; import '@nomicfoundation/hardhat-ethers'; import '@oasisprotocol/sapphire-hardhat'; import '@typechain/hardhat'; -import {JsonRpcProvider} from "ethers"; +import {JsonRpcProvider, TransactionResponse} from "ethers"; import { task } from "hardhat/config"; import {HardhatEthersSigner} from "@nomicfoundation/hardhat-ethers/signers"; import {HardhatRuntimeEnvironment} from "hardhat/types"; @@ -207,7 +207,7 @@ task("upgrade") await hre.run("compile"); const Accounting = await hre.ethers.getContractFactory("Accounting", await getUwDeployer(hre)); - const current = await hre.ethers.getContractAt("Accounting", args.address); + const current = await hre.ethers.getContractAt("Accounting", args.address, await getUwDeployer(hre)); let siweAuthAddress: string = args.siweauth; if (!siweAuthAddress) { @@ -248,35 +248,16 @@ task("upgrade") constructorArgs: [siweAuthAddress], }); - let newImplAddress: string - if (!args.outputSafe) { - const upgraded = await hre.upgrades.upgradeProxy(args.address, Accounting, { - kind: 'uups', - constructorArgs: [siweAuthAddress], - redeployImplementation: 'always', - txOverrides: { gasLimit: 15000000 } - }); - // await upgraded.waitForDeployment(); doesn't work for unwrapped providers. - // Extract the upgrade tx and wait for it directly. - const upgradeTx = (upgraded as unknown as { deployTransaction?: { wait: () => Promise } }).deployTransaction; - await upgradeTx!.wait(); - - newImplAddress = await hre.upgrades.erc1967.getImplementationAddress(args.address); - console.log(`Upgraded! New implementation: ${newImplAddress}`); - - if (currentImpl === newImplAddress) { - console.log(`Warning: Implementation address unchanged. Upgrade may have been a no-op.`); - } - } else { - newImplAddress = await hre.upgrades.prepareUpgrade(args.address, Accounting, { - kind: 'uups', - constructorArgs: [siweAuthAddress], - redeployImplementation: 'always', - txOverrides: { gasLimit: 15000000 } - }) as string; - - console.log(`Deployed new proposed implementation: ${newImplAddress}`); - } + const deployTx = await hre.upgrades.prepareUpgrade(args.address, Accounting, { + kind: 'uups', + constructorArgs: [siweAuthAddress], + redeployImplementation: 'always', + txOverrides: { gasLimit: 15000000 }, + getTxResponse: true, + }) as TransactionResponse; + const deployReceipt = await deployTx.wait(); + const newImplAddress = deployReceipt!.contractAddress!; + console.log(`Deployed new proposed implementation: ${newImplAddress} (tx: ${deployTx.hash})`); try { await hre.run("verify:sourcify", { address: newImplAddress, contract: "Accounting" }); @@ -293,17 +274,39 @@ task("upgrade") ); } - if (args.outputSafe) { - const data = Accounting.interface.encodeFunctionData("upgradeToAndCall", [newImplAddress, "0x"]); + if (!args.outputSafe) { + const txProposeUpgrade = await (await current.proposeUpgrade(newImplAddress, 0)).wait(); + console.log(`Proposed upgrade to ${newImplAddress}. (tx: ${txProposeUpgrade?.hash})`); + + const txUpgradeAndCall = await (await current.upgradeToAndCall(newImplAddress, "0x")).wait(); + console.log(`Upgraded! New implementation: ${newImplAddress}. (tx: ${txUpgradeAndCall?.hash})`); + + const checkImplAddress = await hre.upgrades.erc1967.getImplementationAddress(args.address); + if (checkImplAddress === currentImpl) { + console.log(`Warning: Implementation address unchanged. Upgrade may have been a no-op.`); + } + } else { + const dataProposeUpgrade = Accounting.interface.encodeFunctionData("proposeUpgrade", [newImplAddress, 0]); + const jsonProposeUpgrade = await createSafeJson( + args.address, + dataProposeUpgrade, + "Propose Upgrade of Accounting", + `Propose Upgrade of Accounting contract ${args.address} to implementation ${newImplAddress}`, + (await hre.ethers.provider.getNetwork()).chainId.toString() + ); + writeFileSync(args.outputSafe+"-1", jsonProposeUpgrade); + + const dataUpgradeToAndCall = Accounting.interface.encodeFunctionData("upgradeToAndCall", [newImplAddress, "0x"]); const json = await createSafeJson( args.address, - data, + dataUpgradeToAndCall, "Upgrade Accounting", `Upgrade Accounting contract ${args.address} to implementation ${newImplAddress}`, (await hre.ethers.provider.getNetwork()).chainId.toString() ); - writeFileSync(args.outputSafe, json); - console.log(`Safe Transaction Builder JSON written to ${args.outputSafe}`); + writeFileSync(args.outputSafe+"-2", json); + + console.log(`Two Safe Transaction Builder JSON batches written to ${args.outputSafe}-1 and ${args.outputSafe}-2. Execute them separately.`); } }); diff --git a/solidity/tasks/show.ts b/solidity/tasks/show.ts index b6b4be11..df2f76a1 100644 --- a/solidity/tasks/show.ts +++ b/solidity/tasks/show.ts @@ -22,7 +22,7 @@ task("show") const accounting = await hre.ethers.getContractAt("Accounting", args.address); - const [implAddress, version, owner, siweAuthAddress, withdrawalCount, tokenIds] = + const [implAddress, version, owner, siweAuthAddress, withdrawalCount, tokenIds, proposedUpgradeImpl, proposedUpgradeImplHash, proposedUpgradeMinBlockNumber] = await Promise.all([ tryCall(() => hre.upgrades.erc1967.getImplementationAddress(args.address)), tryCall(() => accounting.VERSION()), @@ -30,8 +30,13 @@ task("show") tryCall(() => accounting.siweAuth()), tryCall(() => accounting.withdrawalCount()), tryCall(() => accounting.getRegisteredTokens()), + tryCall(() => accounting.proposedUpgradeImplementation()), + tryCall(() => accounting.proposedUpgradeImplementationHash()), + tryCall(() => accounting.proposedUpgradeMinBlockNumber()), ]); + const hasProposedUpgrade = proposedUpgradeImpl !== undefined && proposedUpgradeImpl !== hre.ethers.ZeroAddress; + console.log("=== Accounting Contract Info ==="); console.log("Proxy address: ", args.address); console.log("Implementation: ", implAddress); @@ -39,6 +44,12 @@ task("show") console.log("Owner: ", owner); console.log("SiweAuth address: ", siweAuthAddress); console.log("Withdrawal count: ", withdrawalCount?.toString()); + console.log("Proposed upgrade: ", hasProposedUpgrade ? "yes" : "none"); + if (hasProposedUpgrade) { + console.log(" New implementation: ", proposedUpgradeImpl); + console.log(" New implementation hash:", proposedUpgradeImplHash); + console.log(" Min block number: ", proposedUpgradeMinBlockNumber?.toString()); + } console.log(`\n=== Registered Tokens (${tokenIds?.length ?? 0}) ===`); for (const tokenId of tokenIds ?? []) { @@ -46,7 +57,7 @@ task("show") const typeIndex = tokenInfo !== undefined ? Number(tokenInfo.tokenType) : undefined; const typeName = typeIndex !== undefined ? TOKEN_TYPE_NAMES[typeIndex] ?? `Unknown(${typeIndex})` : undefined; - console.log(`\nToken ID: ${tokenId}`); + console.log(`\n Token ID: ${tokenId}`); console.log(` Type: ${typeName}`); if (typeIndex === 0) { @@ -109,5 +120,8 @@ task("show") roflSignerAddress, withdrawalCount: withdrawalCount?.toString(), tokenIds, + proposedUpgradeImpl: hasProposedUpgrade ? proposedUpgradeImpl : undefined, + proposedUpgradeImplHash: hasProposedUpgrade ? proposedUpgradeImplHash : undefined, + proposedUpgradeMinBlockNumber: hasProposedUpgrade ? proposedUpgradeMinBlockNumber?.toString() : undefined, }; }); diff --git a/solidity/test/Accounting.E2E.ts b/solidity/test/Accounting.E2E.ts index 06d7d9f0..2afd20e3 100644 --- a/solidity/test/Accounting.E2E.ts +++ b/solidity/test/Accounting.E2E.ts @@ -4,7 +4,7 @@ import { keccak256, Wallet } from 'ethers'; import { MockAccounting, MockAccountingV2, MockSiweAuth } from '../typechain-types'; import { HardhatNetworkHDAccountsConfig } from 'hardhat/types'; import { HardhatEthersSigner } from '@nomicfoundation/hardhat-ethers/signers'; -import { deployMockAccounting, getDeployer, mockAuthToken, waitForImplementationChange } from './utils'; +import { deployMockAccounting, getDeployer, mockAuthToken } from './utils'; // Mirrors of the Solidity enums in contracts/Types.sol. Typechain exposes enum // parameters as uint8 at the TS boundary, so we use ordinals — kept in sync with @@ -1462,12 +1462,19 @@ describe('Upgradability', function () { const tokenInfoBefore = await accounting.tokens(TEST_TOKEN.tokenId); expect(balanceBefore).to.equal(initialBalance); - // Upgrade to the same implementation (simulates an upgrade) + // Upgrade to the same implementation (simulates an upgrade). + // UPUPSUpgradeable requires proposing the new implementation in one block + // and accepting it in a later block. const AccountingV2Factory = await ethers.getContractFactory('MockAccounting'); - const upgraded = await upgrades.upgradeProxy(proxyAddress, AccountingV2Factory, { + const newImplAddress = await upgrades.prepareUpgrade(proxyAddress, AccountingV2Factory, { kind: 'uups', constructorArgs: [await mockSiweAuth.getAddress()] - }) as unknown as MockAccounting; + }) as string; + + await (await accounting.proposeUpgrade(newImplAddress, 0)).wait(); + await (await accounting.upgradeToAndCall(newImplAddress, "0x")).wait(); + + const upgraded = (await ethers.getContractFactory('MockAccounting')).attach(proxyAddress) as unknown as MockAccounting; // Verify state is preserved after upgrade const balanceAfter = await upgraded.getBalance(user.address, TEST_TOKEN.tokenId); @@ -1575,17 +1582,20 @@ describe('Upgradability', function () { const balanceBefore = await accounting.getBalance(user.address, TEST_TOKEN.tokenId); expect(balanceBefore).to.equal(initialBalance); - // Upgrade to V2 (reinitializer doesn't chain parent inits — they ran in V1) - const implementationBefore = await upgrades.erc1967.getImplementationAddress(proxyAddress); + // Upgrade to V2 (reinitializer doesn't chain parent inits — they ran in V1). + // UPUPSUpgradeable requires proposing the new implementation in one block and + // accepting it in a later block. const AccountingV2Factory = await ethers.getContractFactory('MockAccountingV2'); - const upgraded = await upgrades.upgradeProxy(proxyAddress, AccountingV2Factory, { + const newImplAddress = await upgrades.prepareUpgrade(proxyAddress, AccountingV2Factory, { kind: 'uups', unsafeAllow: ['missing-initializer'], constructorArgs: [await mockSiweAuth.getAddress()], - }) as unknown as MockAccountingV2; + }) as string; + + await (await accounting.proposeUpgrade(newImplAddress, 0)).wait(); + await (await accounting.upgradeToAndCall(newImplAddress, "0x")).wait(); - // sapphire-paratime#688: upgradeProxy may return before the upgrade tx lands. - await waitForImplementationChange(proxyAddress, implementationBefore); + const upgraded = (await ethers.getContractFactory('MockAccountingV2')).attach(proxyAddress) as unknown as MockAccountingV2; // Call reinitializer await (await upgraded.initializeV2(42)).wait();