[eth] Make governance transfer less error-prone (#355)

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;
+    }
 }

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());
     }

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) {
-        uint index = 0;
+    /// @dev Parse a AuthorizeGovernanceDataSourceTransferPayload (action 2) with minimal validation
+    function parseAuthorizeGovernanceDataSourceTransferPayload(bytes memory encodedPayload) public pure returns (AuthorizeGovernanceDataSourceTransferPayload memory sgds) {
+        sgds.claimVaa = encodedPayload;
+    }
 
-        sgds.newGovernanceDataSource.chainId = encodedPayload.toUint16(index);
-        index += 2;
+    /// @dev Parse a AuthorizeGovernanceDataSourceTransferPayload (action 2) with minimal validation
+    function parseRequestGovernanceDataSourceTransferPayload(bytes memory encodedPayload) public pure
+        returns (RequestGovernanceDataSourceTransferPayload memory sgdsClaim) {
 
-        sgds.newGovernanceDataSource.emitterAddress = encodedPayload.toBytes32(index);
-        index += 32;
+        uint index = 0;
 
-        sgds.initialSequence = encodedPayload.toUint64(index);
-        index += 8;
+        sgdsClaim.governanceDataSourceIndex = encodedPayload.toUint32(index);
+        index += 4;
 
-        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

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;
+    }
 }

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;
     }
 }
 

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,
-            testGovernanceChainId, 
-            testGovernanceEmitter,
-            2
-        );
+        expect(newGovernanceDataSource.chainId).equal(newEmitterChain.toString());
+        expect(newGovernanceDataSource.emitterAddress).equal(newEmitterAddress);
 
+        // Verifies the data source has changed.
         await expectRevert(
-            this.pythProxy.executeGovernanceInstruction(newVaaFromOldGovernanceSource),
+            this.pythProxy.executeGovernanceInstruction(vaa),
             "VAA is not coming from the governance data source"
         );
 
-        const newVaaFromNewGovernanceOldSequence = await createVAAFromUint8Array(data,
-            governance.CHAINS.acala, 
-            "0x0000000000000000000000000000000000000000000000000000000000001111",
+        // 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(newVaaFromNewGovernanceOldSequence),
-            "VAA is older than the last executed governance VAA"
+            this.pythProxy.executeGovernanceInstruction(claimLonelyVaa),
+            "RequestGovernanceDataSourceTransfer can be only part of AuthorizeGovernanceDataSourceTransfer message"
         );
 
-        const newVaaFromNewGovernanceGood = await createVAAFromUint8Array(data,
-            governance.CHAINS.acala, 
-            "0x0000000000000000000000000000000000000000000000000000000000001111",
-            20
+        // 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 this.pythProxy.executeGovernanceInstruction(newVaaFromNewGovernanceGood);
+        const transferBackClaimVaaWrong = Buffer.from(transferBackClaimVaaHexStringWrong.substring(2), 'hex');
+
+        const transferBackDataWrong = new governance.AuthorizeGovernanceDataSourceTransferInstruction(
+            governance.CHAINS.unset,
+            transferBackClaimVaaWrong
+        ).serialize();
+
+        const transferBackVaaWrong = await createVAAFromUint8Array(transferBackDataWrong,
+            newEmitterChain, 
+            newEmitterAddress,
+            2
+        );
+
+        await expectRevert(
+            this.pythProxy.executeGovernanceInstruction(transferBackVaaWrong),
+            "cannot upgrade to an older governance data source"
+        );
     });
 
     it("Setting data sources should work", async function () {

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 {

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();
+  }
+}