Skip to content

Commit 398b187

Browse files
ali-behjatiali-behjati
authored
[eth] Make governance transfer less error-prone (#355)
1 parent 808e939 commit 398b187

File tree

8 files changed

+176
-62
lines changed

8 files changed

+176
-62
lines changed

‎ethereum/contracts/pyth/PythGetters.sol

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,4 +63,8 @@ contract PythGetters is PythState {
6363
function validTimePeriodSeconds() public view returns (uint) {
6464
return _state.validTimePeriodSeconds;
6565
}
66+
67+
function governanceDataSourceIndex() public view returns (uint32) {
68+
return _state.governanceDataSourceIndex;
69+
}
6670
}

‎ethereum/contracts/pyth/PythGovernance.sol

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -45,14 +45,16 @@ abstract contract PythGovernance is PythGetters, PythSetters, PythGovernanceInst
4545
if (gi.action == GovernanceAction.UpgradeContract) {
4646
require(gi.targetChainId != 0, "upgrade with chain id 0 is not possible");
4747
upgradeContract(gi.payload);
48-
} else if (gi.action == GovernanceAction.SetGovernanceDataSource) {
49-
setGovernanceDataSource(gi.payload);
48+
} else if (gi.action == GovernanceAction.AuthorizeGovernanceDataSourceTransfer) {
49+
AuthorizeGovernanceDataSourceTransfer(gi.payload);
5050
} else if (gi.action == GovernanceAction.SetDataSources) {
5151
setDataSources(gi.payload);
5252
} else if (gi.action == GovernanceAction.SetFee) {
5353
setFee(gi.payload);
5454
} else if (gi.action == GovernanceAction.SetValidPeriod) {
5555
setValidPeriod(gi.payload);
56+
} else if (gi.action == GovernanceAction.RequestGovernanceDataSourceTransfer) {
57+
revert("RequestGovernanceDataSourceTransfer can be only part of AuthorizeGovernanceDataSourceTransfer message");
5658
} else {
5759
revert("invalid governance action");
5860
}
@@ -67,13 +69,39 @@ abstract contract PythGovernance is PythGetters, PythSetters, PythGovernanceInst
6769

6870
function upgradeUpgradableContract(UpgradeContractPayload memory payload) virtual internal;
6971

70-
function setGovernanceDataSource(bytes memory encodedPayload) internal {
71-
SetGovernanceDataSourcePayload memory payload = parseSetGovernanceDataSourcePayload(encodedPayload);
72-
72+
// Transfer the governance data source to a new value with sanity checks
73+
// to ensure the new governance data source can manage the contract.
74+
function AuthorizeGovernanceDataSourceTransfer(bytes memory encodedPayload) internal {
7375
PythInternalStructs.DataSource memory oldGovernanceDatSource = governanceDataSource();
7476

75-
setGovernanceDataSource(payload.newGovernanceDataSource);
76-
setLastExecutedGovernanceSequence(payload.initialSequence);
77+
AuthorizeGovernanceDataSourceTransferPayload memory payload = parseAuthorizeGovernanceDataSourceTransferPayload(encodedPayload);
78+
79+
// Make sure the claimVaa is a valid VAA with RequestGovernanceDataSourceTransfer governance message
80+
// If it's valid then its emitter can take over the governance from the current emitter.
81+
// The VAA is checked here to ensure that the new governance data source is valid and can send message
82+
// through wormhole.
83+
(IWormhole.VM memory vm, bool valid, string memory reason) = wormhole().parseAndVerifyVM(payload.claimVaa);
84+
require(valid, reason);
85+
86+
GovernanceInstruction memory gi = parseGovernanceInstruction(vm.payload);
87+
require(gi.targetChainId == chainId() || gi.targetChainId == 0, "invalid target chain for this governance instruction");
88+
require(gi.action == GovernanceAction.RequestGovernanceDataSourceTransfer,
89+
"governance data source change inner vaa is not of claim action type");
90+
91+
RequestGovernanceDataSourceTransferPayload memory claimPayload = parseRequestGovernanceDataSourceTransferPayload(gi.payload);
92+
93+
// Governance data source index is used to prevent replay attacks, so a claimVaa cannot be used twice.
94+
require(governanceDataSourceIndex() < claimPayload.governanceDataSourceIndex,
95+
"cannot upgrade to an older governance data source");
96+
97+
setGovernanceDataSourceIndex(claimPayload.governanceDataSourceIndex);
98+
99+
PythInternalStructs.DataSource memory newGovernanceDS = PythInternalStructs.DataSource(vm.emitterChainId, vm.emitterAddress);
100+
101+
setGovernanceDataSource(newGovernanceDS);
102+
103+
// Setting the last executed governance to the claimVaa sequence to avoid using older sequences.
104+
setLastExecutedGovernanceSequence(vm.sequence);
77105

78106
emit GovernanceDataSourceSet(oldGovernanceDatSource, governanceDataSource(), lastExecutedGovernanceSequence());
79107
}

‎ethereum/contracts/pyth/PythGovernanceInstructions.sol

Lines changed: 25 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,11 @@ contract PythGovernanceInstructions {
2626

2727
enum GovernanceAction {
2828
UpgradeContract, // 0
29-
SetGovernanceDataSource, // 1
29+
AuthorizeGovernanceDataSourceTransfer, // 1
3030
SetDataSources, // 2
3131
SetFee, // 3
32-
SetValidPeriod // 4
32+
SetValidPeriod, // 4
33+
RequestGovernanceDataSourceTransfer // 5
3334
}
3435

3536
struct GovernanceInstruction {
@@ -43,9 +44,17 @@ contract PythGovernanceInstructions {
4344
address newImplementation;
4445
}
4546

46-
struct SetGovernanceDataSourcePayload {
47-
PythInternalStructs.DataSource newGovernanceDataSource;
48-
uint64 initialSequence;
47+
struct AuthorizeGovernanceDataSourceTransferPayload {
48+
// Transfer governance control over this contract to another data source.
49+
// The claimVaa field is a VAA created by the new data source; using a VAA prevents mistakes
50+
// in the handoff by ensuring that the new data source can send VAAs (i.e., is not an invalid address).
51+
bytes claimVaa;
52+
}
53+
54+
struct RequestGovernanceDataSourceTransferPayload {
55+
// Governance data source index is used to prevent replay attacks
56+
// So a claimVaa cannot be used twice.
57+
uint32 governanceDataSourceIndex;
4958
}
5059

5160
struct SetDataSourcesPayload {
@@ -94,20 +103,21 @@ contract PythGovernanceInstructions {
94103
require(encodedPayload.length == index, "invalid length for UpgradeContractPayload");
95104
}
96105

97-
/// @dev Parse a SetGovernanceDataSourcePayload (action 2) with minimal validation
98-
function parseSetGovernanceDataSourcePayload(bytes memory encodedPayload) public pure returns (SetGovernanceDataSourcePayload memory sgds) {
99-
uint index = 0;
106+
/// @dev Parse a AuthorizeGovernanceDataSourceTransferPayload (action 2) with minimal validation
107+
function parseAuthorizeGovernanceDataSourceTransferPayload(bytes memory encodedPayload) public pure returns (AuthorizeGovernanceDataSourceTransferPayload memory sgds) {
108+
sgds.claimVaa = encodedPayload;
109+
}
100110

101-
sgds.newGovernanceDataSource.chainId = encodedPayload.toUint16(index);
102-
index += 2;
111+
/// @dev Parse a AuthorizeGovernanceDataSourceTransferPayload (action 2) with minimal validation
112+
function parseRequestGovernanceDataSourceTransferPayload(bytes memory encodedPayload) public pure
113+
returns (RequestGovernanceDataSourceTransferPayload memory sgdsClaim) {
103114

104-
sgds.newGovernanceDataSource.emitterAddress = encodedPayload.toBytes32(index);
105-
index += 32;
115+
uint index = 0;
106116

107-
sgds.initialSequence = encodedPayload.toUint64(index);
108-
index += 8;
117+
sgdsClaim.governanceDataSourceIndex = encodedPayload.toUint32(index);
118+
index += 4;
109119

110-
require(encodedPayload.length == index, "invalid length for SetGovernanceDataSourcePayload");
120+
require(encodedPayload.length == index, "invalid length for RequestGovernanceDataSourceTransferPayload");
111121
}
112122

113123
/// @dev Parse a SetDataSourcesPayload (action 3) with minimal validation

‎ethereum/contracts/pyth/PythSetters.sol

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,4 +37,8 @@ contract PythSetters is PythState {
3737
function setLastExecutedGovernanceSequence(uint64 sequence) internal {
3838
_state.lastExecutedGovernanceSequence = sequence;
3939
}
40+
41+
function setGovernanceDataSourceIndex(uint32 newIndex) internal {
42+
_state.governanceDataSourceIndex = newIndex;
43+
}
4044
}

‎ethereum/contracts/pyth/PythState.sol

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,10 @@ contract PythStorage {
3939
// Mapping of cached price information
4040
// priceId => PriceInfo
4141
mapping(bytes32 => PythInternalStructs.PriceInfo) latestPriceInfo;
42+
43+
// Index of the governance data source, increased each time the governance data source
44+
// changes.
45+
uint32 governanceDataSourceIndex;
4246
}
4347
}
4448

‎ethereum/test/pyth.js

Lines changed: 79 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1004,16 +1004,32 @@ contract("Pyth", function () {
10041004
);
10051005
});
10061006

1007-
it("Setting governance data source should work", async function () {
1008-
const data = new governance.SetGovernanceDataSourceInstruction(
1009-
governance.CHAINS.ethereum,
1010-
new governance.DataSource(
1011-
governance.CHAINS.acala,
1012-
new governance.HexString32Bytes(
1013-
"0x0000000000000000000000000000000000000000000000000000000000001111",
1014-
)
1015-
),
1016-
BigInt(10)
1007+
it("Transferring governance data source should work", async function () {
1008+
const newEmitterAddress = "0x0000000000000000000000000000000000000000000000000000000000001111";
1009+
const newEmitterChain = governance.CHAINS.acala;
1010+
1011+
const claimInstructionData = new governance.RequestGovernanceDataSourceTransferInstruction(
1012+
governance.CHAINS.unset,
1013+
1
1014+
).serialize();
1015+
1016+
const claimVaaHexString = await createVAAFromUint8Array(
1017+
claimInstructionData,
1018+
newEmitterChain,
1019+
newEmitterAddress,
1020+
1
1021+
);
1022+
1023+
await expectRevert(
1024+
this.pythProxy.executeGovernanceInstruction(claimVaaHexString),
1025+
"VAA is not coming from the governance data source"
1026+
);
1027+
1028+
const claimVaa = Buffer.from(claimVaaHexString.substring(2), 'hex');
1029+
1030+
const data = new governance.AuthorizeGovernanceDataSourceTransferInstruction(
1031+
governance.CHAINS.unset,
1032+
claimVaa
10171033
).serialize();
10181034

10191035
const vaa = await createVAAFromUint8Array(data,
@@ -1025,40 +1041,75 @@ contract("Pyth", function () {
10251041
const oldGovernanceDataSource = await this.pythProxy.governanceDataSource();
10261042

10271043
const receipt = await this.pythProxy.executeGovernanceInstruction(vaa);
1044+
1045+
const newGovernanceDataSource = await this.pythProxy.governanceDataSource();
1046+
10281047
expectEvent(receipt, 'GovernanceDataSourceSet', {
10291048
oldDataSource: oldGovernanceDataSource,
1030-
newDataSource: await this.pythProxy.governanceDataSource(),
1049+
newDataSource: newGovernanceDataSource,
10311050
});
10321051

1033-
const newVaaFromOldGovernanceSource = await createVAAFromUint8Array(data,
1034-
testGovernanceChainId,
1035-
testGovernanceEmitter,
1036-
2
1037-
);
1052+
expect(newGovernanceDataSource.chainId).equal(newEmitterChain.toString());
1053+
expect(newGovernanceDataSource.emitterAddress).equal(newEmitterAddress);
10381054

1055+
// Verifies the data source has changed.
10391056
await expectRevert(
1040-
this.pythProxy.executeGovernanceInstruction(newVaaFromOldGovernanceSource),
1057+
this.pythProxy.executeGovernanceInstruction(vaa),
10411058
"VAA is not coming from the governance data source"
10421059
);
10431060

1044-
const newVaaFromNewGovernanceOldSequence = await createVAAFromUint8Array(data,
1045-
governance.CHAINS.acala,
1046-
"0x0000000000000000000000000000000000000000000000000000000000001111",
1061+
// Make sure a claim vaa does not get executed
1062+
1063+
const claimLonely = new governance.RequestGovernanceDataSourceTransferInstruction(
1064+
governance.CHAINS.unset,
10471065
2
1048-
);
1066+
).serialize();
10491067

1068+
const claimLonelyVaa = await createVAAFromUint8Array(
1069+
claimLonely,
1070+
newEmitterChain,
1071+
newEmitterAddress,
1072+
2
1073+
);
1074+
10501075
await expectRevert(
1051-
this.pythProxy.executeGovernanceInstruction(newVaaFromNewGovernanceOldSequence),
1052-
"VAA is older than the last executed governance VAA"
1076+
this.pythProxy.executeGovernanceInstruction(claimLonelyVaa),
1077+
"RequestGovernanceDataSourceTransfer can be only part of AuthorizeGovernanceDataSourceTransfer message"
10531078
);
10541079

1055-
const newVaaFromNewGovernanceGood = await createVAAFromUint8Array(data,
1056-
governance.CHAINS.acala,
1057-
"0x0000000000000000000000000000000000000000000000000000000000001111",
1058-
20
1080+
// Transfer back the ownership to the old governance data source without increasing
1081+
// the governance index should not work
1082+
1083+
// A wrong vaa that does not move the governance index
1084+
const transferBackClaimInstructionDataWrong = new governance.RequestGovernanceDataSourceTransferInstruction(
1085+
governance.CHAINS.unset,
1086+
1 // The same governance data source index => Should fail
1087+
).serialize();
1088+
1089+
const transferBackClaimVaaHexStringWrong = await createVAAFromUint8Array(
1090+
transferBackClaimInstructionDataWrong,
1091+
testGovernanceChainId,
1092+
testGovernanceEmitter,
1093+
2
10591094
);
10601095

1061-
await this.pythProxy.executeGovernanceInstruction(newVaaFromNewGovernanceGood);
1096+
const transferBackClaimVaaWrong = Buffer.from(transferBackClaimVaaHexStringWrong.substring(2), 'hex');
1097+
1098+
const transferBackDataWrong = new governance.AuthorizeGovernanceDataSourceTransferInstruction(
1099+
governance.CHAINS.unset,
1100+
transferBackClaimVaaWrong
1101+
).serialize();
1102+
1103+
const transferBackVaaWrong = await createVAAFromUint8Array(transferBackDataWrong,
1104+
newEmitterChain,
1105+
newEmitterAddress,
1106+
2
1107+
);
1108+
1109+
await expectRevert(
1110+
this.pythProxy.executeGovernanceInstruction(transferBackVaaWrong),
1111+
"cannot upgrade to an older governance data source"
1112+
);
10621113
});
10631114

10641115
it("Setting data sources should work", async function () {

‎third_party/pyth/xc-governance-sdk-js/src/index.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,9 @@ export {
66
HexString32Bytes,
77
SetDataSourcesInstruction,
88
SetFeeInstruction,
9-
SetGovernanceDataSourceInstruction,
10-
SetValidPeriodInstruction
9+
SetValidPeriodInstruction,
10+
RequestGovernanceDataSourceTransferInstruction,
11+
AuthorizeGovernanceDataSourceTransferInstruction
1112
} from "./instructions"
1213

1314
export {

‎third_party/pyth/xc-governance-sdk-js/src/instructions.ts

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { ChainId } from "@certusone/wormhole-sdk";
1+
import { ChainId, CHAINS } from "@certusone/wormhole-sdk";
22

33
import { Serializable, BufferBuilder } from "./serialize";
44

@@ -9,10 +9,11 @@ enum Module {
99

1010
enum TargetAction {
1111
UpgradeContract = 0,
12-
SetGovernanceDataSource,
12+
AuthorizeGovernanceDataSourceTransfer,
1313
SetDataSources,
1414
SetFee,
1515
SetValidPeriod,
16+
RequestGovernanceDataSourceTransfer,
1617
}
1718

1819
abstract class HexString implements Serializable {
@@ -126,20 +127,16 @@ export class EthereumUpgradeContractInstruction extends TargetInstruction {
126127
}
127128
}
128129

129-
export class SetGovernanceDataSourceInstruction extends TargetInstruction {
130+
export class AuthorizeGovernanceDataSourceTransferInstruction extends TargetInstruction {
130131
constructor(
131132
targetChainId: ChainId,
132-
private governanceDataSource: DataSource,
133-
private initialSequence: bigint,
133+
private claimVaa: Buffer,
134134
) {
135-
super(TargetAction.SetGovernanceDataSource, targetChainId);
135+
super(TargetAction.AuthorizeGovernanceDataSourceTransfer, targetChainId);
136136
}
137137

138138
protected serializePayload(): Buffer {
139-
return new BufferBuilder()
140-
.addObject(this.governanceDataSource)
141-
.addBigUint64(this.initialSequence)
142-
.build();
139+
return this.claimVaa;
143140
}
144141
}
145142

@@ -190,3 +187,18 @@ export class SetValidPeriodInstruction extends TargetInstruction {
190187
.build();
191188
}
192189
}
190+
191+
export class RequestGovernanceDataSourceTransferInstruction extends TargetInstruction {
192+
constructor(
193+
targetChainId: ChainId,
194+
private governanceDataSourceIndex: number,
195+
) {
196+
super(TargetAction.RequestGovernanceDataSourceTransfer, targetChainId);
197+
}
198+
199+
protected serializePayload(): Buffer {
200+
return new BufferBuilder()
201+
.addUint32(this.governanceDataSourceIndex)
202+
.build();
203+
}
204+
}

0 commit comments

Comments
 (0)