diff --git a/ethereum/contracts/pyth/PythGetters.sol b/ethereum/contracts/pyth/PythGetters.sol index 09d523ba..3fa56f56 100644 --- a/ethereum/contracts/pyth/PythGetters.sol +++ b/ethereum/contracts/pyth/PythGetters.sol @@ -63,4 +63,8 @@ contract PythGetters is PythState { function validTimePeriodSeconds() public view returns (uint) { return _state.validTimePeriodSeconds; } + + function governanceDataSourceIndex() public view returns (uint32) { + return _state.governanceDataSourceIndex; + } } diff --git a/ethereum/contracts/pyth/PythGovernance.sol b/ethereum/contracts/pyth/PythGovernance.sol index 18d516a5..054c8793 100644 --- a/ethereum/contracts/pyth/PythGovernance.sol +++ b/ethereum/contracts/pyth/PythGovernance.sol @@ -45,14 +45,16 @@ abstract contract PythGovernance is PythGetters, PythSetters, PythGovernanceInst if (gi.action == GovernanceAction.UpgradeContract) { require(gi.targetChainId != 0, "upgrade with chain id 0 is not possible"); upgradeContract(gi.payload); - } else if (gi.action == GovernanceAction.SetGovernanceDataSource) { - setGovernanceDataSource(gi.payload); + } else if (gi.action == GovernanceAction.AuthorizeGovernanceDataSourceTransfer) { + AuthorizeGovernanceDataSourceTransfer(gi.payload); } else if (gi.action == GovernanceAction.SetDataSources) { setDataSources(gi.payload); } else if (gi.action == GovernanceAction.SetFee) { setFee(gi.payload); } else if (gi.action == GovernanceAction.SetValidPeriod) { setValidPeriod(gi.payload); + } else if (gi.action == GovernanceAction.RequestGovernanceDataSourceTransfer) { + revert("RequestGovernanceDataSourceTransfer can be only part of AuthorizeGovernanceDataSourceTransfer message"); } else { revert("invalid governance action"); } @@ -67,13 +69,39 @@ abstract contract PythGovernance is PythGetters, PythSetters, PythGovernanceInst function upgradeUpgradableContract(UpgradeContractPayload memory payload) virtual internal; - function setGovernanceDataSource(bytes memory encodedPayload) internal { - SetGovernanceDataSourcePayload memory payload = parseSetGovernanceDataSourcePayload(encodedPayload); - + // Transfer the governance data source to a new value with sanity checks + // to ensure the new governance data source can manage the contract. + function AuthorizeGovernanceDataSourceTransfer(bytes memory encodedPayload) internal { PythInternalStructs.DataSource memory oldGovernanceDatSource = governanceDataSource(); - setGovernanceDataSource(payload.newGovernanceDataSource); - setLastExecutedGovernanceSequence(payload.initialSequence); + AuthorizeGovernanceDataSourceTransferPayload memory payload = parseAuthorizeGovernanceDataSourceTransferPayload(encodedPayload); + + // Make sure the claimVaa is a valid VAA with RequestGovernanceDataSourceTransfer governance message + // If it's valid then its emitter can take over the governance from the current emitter. + // The VAA is checked here to ensure that the new governance data source is valid and can send message + // through wormhole. + (IWormhole.VM memory vm, bool valid, string memory reason) = wormhole().parseAndVerifyVM(payload.claimVaa); + require(valid, reason); + + GovernanceInstruction memory gi = parseGovernanceInstruction(vm.payload); + require(gi.targetChainId == chainId() || gi.targetChainId == 0, "invalid target chain for this governance instruction"); + require(gi.action == GovernanceAction.RequestGovernanceDataSourceTransfer, + "governance data source change inner vaa is not of claim action type"); + + RequestGovernanceDataSourceTransferPayload memory claimPayload = parseRequestGovernanceDataSourceTransferPayload(gi.payload); + + // Governance data source index is used to prevent replay attacks, so a claimVaa cannot be used twice. + require(governanceDataSourceIndex() < claimPayload.governanceDataSourceIndex, + "cannot upgrade to an older governance data source"); + + setGovernanceDataSourceIndex(claimPayload.governanceDataSourceIndex); + + PythInternalStructs.DataSource memory newGovernanceDS = PythInternalStructs.DataSource(vm.emitterChainId, vm.emitterAddress); + + setGovernanceDataSource(newGovernanceDS); + + // Setting the last executed governance to the claimVaa sequence to avoid using older sequences. + setLastExecutedGovernanceSequence(vm.sequence); emit GovernanceDataSourceSet(oldGovernanceDatSource, governanceDataSource(), lastExecutedGovernanceSequence()); } diff --git a/ethereum/contracts/pyth/PythGovernanceInstructions.sol b/ethereum/contracts/pyth/PythGovernanceInstructions.sol index ed122efa..316bfadb 100644 --- a/ethereum/contracts/pyth/PythGovernanceInstructions.sol +++ b/ethereum/contracts/pyth/PythGovernanceInstructions.sol @@ -26,10 +26,11 @@ contract PythGovernanceInstructions { enum GovernanceAction { UpgradeContract, // 0 - SetGovernanceDataSource, // 1 + AuthorizeGovernanceDataSourceTransfer, // 1 SetDataSources, // 2 SetFee, // 3 - SetValidPeriod // 4 + SetValidPeriod, // 4 + RequestGovernanceDataSourceTransfer // 5 } struct GovernanceInstruction { @@ -43,9 +44,17 @@ contract PythGovernanceInstructions { address newImplementation; } - struct SetGovernanceDataSourcePayload { - PythInternalStructs.DataSource newGovernanceDataSource; - uint64 initialSequence; + struct AuthorizeGovernanceDataSourceTransferPayload { + // Transfer governance control over this contract to another data source. + // The claimVaa field is a VAA created by the new data source; using a VAA prevents mistakes + // in the handoff by ensuring that the new data source can send VAAs (i.e., is not an invalid address). + bytes claimVaa; + } + + struct RequestGovernanceDataSourceTransferPayload { + // Governance data source index is used to prevent replay attacks + // So a claimVaa cannot be used twice. + uint32 governanceDataSourceIndex; } struct SetDataSourcesPayload { @@ -94,20 +103,21 @@ contract PythGovernanceInstructions { require(encodedPayload.length == index, "invalid length for UpgradeContractPayload"); } - /// @dev Parse a SetGovernanceDataSourcePayload (action 2) with minimal validation - function parseSetGovernanceDataSourcePayload(bytes memory encodedPayload) public pure returns (SetGovernanceDataSourcePayload memory sgds) { + /// @dev Parse a AuthorizeGovernanceDataSourceTransferPayload (action 2) with minimal validation + function parseAuthorizeGovernanceDataSourceTransferPayload(bytes memory encodedPayload) public pure returns (AuthorizeGovernanceDataSourceTransferPayload memory sgds) { + sgds.claimVaa = encodedPayload; + } + + /// @dev Parse a AuthorizeGovernanceDataSourceTransferPayload (action 2) with minimal validation + function parseRequestGovernanceDataSourceTransferPayload(bytes memory encodedPayload) public pure + returns (RequestGovernanceDataSourceTransferPayload memory sgdsClaim) { + uint index = 0; - sgds.newGovernanceDataSource.chainId = encodedPayload.toUint16(index); - index += 2; + sgdsClaim.governanceDataSourceIndex = encodedPayload.toUint32(index); + index += 4; - sgds.newGovernanceDataSource.emitterAddress = encodedPayload.toBytes32(index); - index += 32; - - sgds.initialSequence = encodedPayload.toUint64(index); - index += 8; - - require(encodedPayload.length == index, "invalid length for SetGovernanceDataSourcePayload"); + require(encodedPayload.length == index, "invalid length for RequestGovernanceDataSourceTransferPayload"); } /// @dev Parse a SetDataSourcesPayload (action 3) with minimal validation diff --git a/ethereum/contracts/pyth/PythSetters.sol b/ethereum/contracts/pyth/PythSetters.sol index e6b41d85..a571fc60 100644 --- a/ethereum/contracts/pyth/PythSetters.sol +++ b/ethereum/contracts/pyth/PythSetters.sol @@ -37,4 +37,8 @@ contract PythSetters is PythState { function setLastExecutedGovernanceSequence(uint64 sequence) internal { _state.lastExecutedGovernanceSequence = sequence; } + + function setGovernanceDataSourceIndex(uint32 newIndex) internal { + _state.governanceDataSourceIndex = newIndex; + } } diff --git a/ethereum/contracts/pyth/PythState.sol b/ethereum/contracts/pyth/PythState.sol index 8198e1ec..9482ab57 100644 --- a/ethereum/contracts/pyth/PythState.sol +++ b/ethereum/contracts/pyth/PythState.sol @@ -39,6 +39,10 @@ contract PythStorage { // Mapping of cached price information // priceId => PriceInfo mapping(bytes32 => PythInternalStructs.PriceInfo) latestPriceInfo; + + // Index of the governance data source, increased each time the governance data source + // changes. + uint32 governanceDataSourceIndex; } } diff --git a/ethereum/test/pyth.js b/ethereum/test/pyth.js index 6806a7a4..7e206811 100644 --- a/ethereum/test/pyth.js +++ b/ethereum/test/pyth.js @@ -1004,16 +1004,32 @@ contract("Pyth", function () { ); }); - it("Setting governance data source should work", async function () { - const data = new governance.SetGovernanceDataSourceInstruction( - governance.CHAINS.ethereum, - new governance.DataSource( - governance.CHAINS.acala, - new governance.HexString32Bytes( - "0x0000000000000000000000000000000000000000000000000000000000001111", - ) - ), - BigInt(10) + it("Transferring governance data source should work", async function () { + const newEmitterAddress = "0x0000000000000000000000000000000000000000000000000000000000001111"; + const newEmitterChain = governance.CHAINS.acala; + + const claimInstructionData = new governance.RequestGovernanceDataSourceTransferInstruction( + governance.CHAINS.unset, + 1 + ).serialize(); + + const claimVaaHexString = await createVAAFromUint8Array( + claimInstructionData, + newEmitterChain, + newEmitterAddress, + 1 + ); + + await expectRevert( + this.pythProxy.executeGovernanceInstruction(claimVaaHexString), + "VAA is not coming from the governance data source" + ); + + const claimVaa = Buffer.from(claimVaaHexString.substring(2), 'hex'); + + const data = new governance.AuthorizeGovernanceDataSourceTransferInstruction( + governance.CHAINS.unset, + claimVaa ).serialize(); const vaa = await createVAAFromUint8Array(data, @@ -1025,40 +1041,75 @@ contract("Pyth", function () { const oldGovernanceDataSource = await this.pythProxy.governanceDataSource(); const receipt = await this.pythProxy.executeGovernanceInstruction(vaa); + + const newGovernanceDataSource = await this.pythProxy.governanceDataSource(); + expectEvent(receipt, 'GovernanceDataSourceSet', { oldDataSource: oldGovernanceDataSource, - newDataSource: await this.pythProxy.governanceDataSource(), + newDataSource: newGovernanceDataSource, }); - const newVaaFromOldGovernanceSource = await createVAAFromUint8Array(data, + expect(newGovernanceDataSource.chainId).equal(newEmitterChain.toString()); + expect(newGovernanceDataSource.emitterAddress).equal(newEmitterAddress); + + // Verifies the data source has changed. + await expectRevert( + this.pythProxy.executeGovernanceInstruction(vaa), + "VAA is not coming from the governance data source" + ); + + // Make sure a claim vaa does not get executed + + const claimLonely = new governance.RequestGovernanceDataSourceTransferInstruction( + governance.CHAINS.unset, + 2 + ).serialize(); + + const claimLonelyVaa = await createVAAFromUint8Array( + claimLonely, + newEmitterChain, + newEmitterAddress, + 2 + ); + + await expectRevert( + this.pythProxy.executeGovernanceInstruction(claimLonelyVaa), + "RequestGovernanceDataSourceTransfer can be only part of AuthorizeGovernanceDataSourceTransfer message" + ); + + // Transfer back the ownership to the old governance data source without increasing + // the governance index should not work + + // A wrong vaa that does not move the governance index + const transferBackClaimInstructionDataWrong = new governance.RequestGovernanceDataSourceTransferInstruction( + governance.CHAINS.unset, + 1 // The same governance data source index => Should fail + ).serialize(); + + const transferBackClaimVaaHexStringWrong = await createVAAFromUint8Array( + transferBackClaimInstructionDataWrong, testGovernanceChainId, testGovernanceEmitter, 2 ); - await expectRevert( - this.pythProxy.executeGovernanceInstruction(newVaaFromOldGovernanceSource), - "VAA is not coming from the governance data source" - ); + const transferBackClaimVaaWrong = Buffer.from(transferBackClaimVaaHexStringWrong.substring(2), 'hex'); - const newVaaFromNewGovernanceOldSequence = await createVAAFromUint8Array(data, - governance.CHAINS.acala, - "0x0000000000000000000000000000000000000000000000000000000000001111", + const transferBackDataWrong = new governance.AuthorizeGovernanceDataSourceTransferInstruction( + governance.CHAINS.unset, + transferBackClaimVaaWrong + ).serialize(); + + const transferBackVaaWrong = await createVAAFromUint8Array(transferBackDataWrong, + newEmitterChain, + newEmitterAddress, 2 ); await expectRevert( - this.pythProxy.executeGovernanceInstruction(newVaaFromNewGovernanceOldSequence), - "VAA is older than the last executed governance VAA" + this.pythProxy.executeGovernanceInstruction(transferBackVaaWrong), + "cannot upgrade to an older governance data source" ); - - const newVaaFromNewGovernanceGood = await createVAAFromUint8Array(data, - governance.CHAINS.acala, - "0x0000000000000000000000000000000000000000000000000000000000001111", - 20 - ); - - await this.pythProxy.executeGovernanceInstruction(newVaaFromNewGovernanceGood); }); it("Setting data sources should work", async function () { diff --git a/third_party/pyth/xc-governance-sdk-js/src/index.ts b/third_party/pyth/xc-governance-sdk-js/src/index.ts index e1b3c349..4793fdf6 100644 --- a/third_party/pyth/xc-governance-sdk-js/src/index.ts +++ b/third_party/pyth/xc-governance-sdk-js/src/index.ts @@ -6,8 +6,9 @@ export { HexString32Bytes, SetDataSourcesInstruction, SetFeeInstruction, - SetGovernanceDataSourceInstruction, - SetValidPeriodInstruction + SetValidPeriodInstruction, + RequestGovernanceDataSourceTransferInstruction, + AuthorizeGovernanceDataSourceTransferInstruction } from "./instructions" export { diff --git a/third_party/pyth/xc-governance-sdk-js/src/instructions.ts b/third_party/pyth/xc-governance-sdk-js/src/instructions.ts index 0d76ca5a..b03c3c5b 100644 --- a/third_party/pyth/xc-governance-sdk-js/src/instructions.ts +++ b/third_party/pyth/xc-governance-sdk-js/src/instructions.ts @@ -1,4 +1,4 @@ -import { ChainId } from "@certusone/wormhole-sdk"; +import { ChainId, CHAINS } from "@certusone/wormhole-sdk"; import { Serializable, BufferBuilder } from "./serialize"; @@ -9,10 +9,11 @@ enum Module { enum TargetAction { UpgradeContract = 0, - SetGovernanceDataSource, + AuthorizeGovernanceDataSourceTransfer, SetDataSources, SetFee, SetValidPeriod, + RequestGovernanceDataSourceTransfer, } abstract class HexString implements Serializable { @@ -126,20 +127,16 @@ export class EthereumUpgradeContractInstruction extends TargetInstruction { } } -export class SetGovernanceDataSourceInstruction extends TargetInstruction { +export class AuthorizeGovernanceDataSourceTransferInstruction extends TargetInstruction { constructor( targetChainId: ChainId, - private governanceDataSource: DataSource, - private initialSequence: bigint, + private claimVaa: Buffer, ) { - super(TargetAction.SetGovernanceDataSource, targetChainId); + super(TargetAction.AuthorizeGovernanceDataSourceTransfer, targetChainId); } protected serializePayload(): Buffer { - return new BufferBuilder() - .addObject(this.governanceDataSource) - .addBigUint64(this.initialSequence) - .build(); + return this.claimVaa; } } @@ -190,3 +187,18 @@ export class SetValidPeriodInstruction extends TargetInstruction { .build(); } } + +export class RequestGovernanceDataSourceTransferInstruction extends TargetInstruction { + constructor( + targetChainId: ChainId, + private governanceDataSourceIndex: number, + ) { + super(TargetAction.RequestGovernanceDataSourceTransfer, targetChainId); + } + + protected serializePayload(): Buffer { + return new BufferBuilder() + .addUint32(this.governanceDataSourceIndex) + .build(); + } +}