Skip to content

Commit c3461e5

Browse files
ali-behjatiali-behjati
authored
[eth] Use PythErrors everywhere (#404)
* Remove unnecessary check * Use PythErrors everywhere
1 parent b31f768 commit c3461e5

File tree

9 files changed

+164
-214
lines changed

9 files changed

+164
-214
lines changed

‎ethereum/contracts/pyth/Pyth.sol‎

Lines changed: 36 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import "../libraries/external/UnsafeBytesLib.sol";
77
import "@pythnetwork/pyth-sdk-solidity/AbstractPyth.sol";
88
import "@pythnetwork/pyth-sdk-solidity/PythStructs.sol";
99

10+
import "@pythnetwork/pyth-sdk-solidity/PythErrors.sol";
1011
import "./PythGetters.sol";
1112
import "./PythSetters.sol";
1213
import "./PythInternalStructs.sol";
@@ -21,11 +22,10 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
2122
) internal {
2223
setWormhole(wormhole);
2324

24-
require(
25-
dataSourceEmitterChainIds.length ==
26-
dataSourceEmitterAddresses.length,
27-
"data source arguments should have the same length"
28-
);
25+
if (
26+
dataSourceEmitterChainIds.length !=
27+
dataSourceEmitterAddresses.length
28+
) revert PythErrors.InvalidArgument();
2929

3030
for (uint i = 0; i < dataSourceEmitterChainIds.length; i++) {
3131
PythInternalStructs.DataSource memory ds = PythInternalStructs
@@ -34,10 +34,8 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
3434
dataSourceEmitterAddresses[i]
3535
);
3636

37-
require(
38-
!PythGetters.isValidDataSource(ds.chainId, ds.emitterAddress),
39-
"Data source already added"
40-
);
37+
if (PythGetters.isValidDataSource(ds.chainId, ds.emitterAddress))
38+
revert PythErrors.InvalidArgument();
4139

4240
_state.isValidDataSource[hashDataSource(ds)] = true;
4341
_state.validDataSources.push(ds);
@@ -57,7 +55,7 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
5755
bytes[] calldata updateData
5856
) public payable override {
5957
uint requiredFee = getUpdateFee(updateData);
60-
require(msg.value >= requiredFee, "insufficient paid fee amount");
58+
if (msg.value < requiredFee) revert PythErrors.InsufficientFee();
6159

6260
for (uint i = 0; i < updateData.length; ) {
6361
updatePriceBatchFromVm(updateData[i]);
@@ -245,10 +243,8 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
245243
}
246244
}
247245

248-
require(
249-
attestationIndex <= attestationSize,
250-
"INTERNAL: Consumed more than `attestationSize` bytes"
251-
);
246+
if (attestationIndex > attestationSize)
247+
revert PythErrors.InvalidUpdateData();
252248
}
253249
}
254250

@@ -259,10 +255,8 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
259255
bytes32[] calldata priceIds,
260256
uint64[] calldata publishTimes
261257
) external payable override {
262-
require(
263-
priceIds.length == publishTimes.length,
264-
"priceIds and publishTimes arrays should have same length"
265-
);
258+
if (priceIds.length != publishTimes.length)
259+
revert PythErrors.InvalidArgument();
266260

267261
for (uint i = 0; i < priceIds.length; ) {
268262
// If the price does not exist, then the publish time is zero and
@@ -277,9 +271,7 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
277271
}
278272
}
279273

280-
revert(
281-
"no prices in the submitted batch have fresh prices, so this update will have no effect"
282-
);
274+
revert PythErrors.NoFreshUpdate();
283275
}
284276

285277
// This is an overwrite of the same method in AbstractPyth.sol
@@ -295,10 +287,7 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
295287
price.price = info.price;
296288
price.conf = info.conf;
297289

298-
require(
299-
price.publishTime != 0,
300-
"price feed for the given id is not pushed or does not exist"
301-
);
290+
if (price.publishTime == 0) revert PythErrors.PriceFeedNotFound();
302291
}
303292

304293
// This is an overwrite of the same method in AbstractPyth.sol
@@ -314,10 +303,7 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
314303
price.price = info.emaPrice;
315304
price.conf = info.emaConf;
316305

317-
require(
318-
price.publishTime != 0,
319-
"price feed for the given id is not pushed or does not exist"
320-
);
306+
if (price.publishTime == 0) revert PythErrors.PriceFeedNotFound();
321307
}
322308

323309
function parseBatchAttestationHeader(
@@ -334,18 +320,20 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
334320
{
335321
uint32 magic = UnsafeBytesLib.toUint32(encoded, index);
336322
index += 4;
337-
require(magic == 0x50325748, "invalid magic value");
323+
if (magic != 0x50325748) revert PythErrors.InvalidUpdateData();
338324

339325
uint16 versionMajor = UnsafeBytesLib.toUint16(encoded, index);
340326
index += 2;
341-
require(versionMajor == 3, "invalid version major, expected 3");
327+
if (versionMajor != 3) revert PythErrors.InvalidUpdateData();
342328

343-
uint16 versionMinor = UnsafeBytesLib.toUint16(encoded, index);
329+
// This value is only used as the check below which currently
330+
// never reverts
331+
// uint16 versionMinor = UnsafeBytesLib.toUint16(encoded, index);
344332
index += 2;
345-
require(
346-
versionMinor >= 0,
347-
"invalid version minor, expected 0 or more"
348-
);
333+
334+
// This check is always false as versionMinor is 0, so it is commented.
335+
// in the future that the minor version increases this will have effect.
336+
// if(versionMinor < 0) revert InvalidUpdateData();
349337

350338
uint16 hdrSize = UnsafeBytesLib.toUint16(encoded, index);
351339
index += 2;
@@ -370,10 +358,7 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
370358
index += hdrSize;
371359

372360
// Payload ID of 2 required for batch headerBa
373-
require(
374-
payloadId == 2,
375-
"invalid payload ID, expected 2 for BatchPriceAttestation"
376-
);
361+
if (payloadId != 2) revert PythErrors.InvalidUpdateData();
377362
}
378363

379364
// Parse the number of attestations
@@ -386,10 +371,8 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
386371

387372
// Given the message is valid the arithmetic below should not overflow, and
388373
// even if it overflows then the require would fail.
389-
require(
390-
encoded.length == (index + (attestationSize * nAttestations)),
391-
"invalid BatchPriceAttestation size"
392-
);
374+
if (encoded.length != (index + (attestationSize * nAttestations)))
375+
revert PythErrors.InvalidUpdateData();
393376
}
394377
}
395378

@@ -398,12 +381,11 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
398381
) internal view returns (IWormhole.VM memory vm) {
399382
{
400383
bool valid;
401-
string memory reason;
402-
(vm, valid, reason) = wormhole().parseAndVerifyVM(encodedVm);
403-
require(valid, reason);
384+
(vm, valid, ) = wormhole().parseAndVerifyVM(encodedVm);
385+
if (!valid) revert PythErrors.InvalidWormholeVaa();
404386
}
405387

406-
require(verifyPythVM(vm), "invalid data source chain/emitter ID");
388+
if (!verifyPythVM(vm)) revert PythErrors.InvalidUpdateDataSource();
407389
}
408390

409391
function parsePriceFeedUpdates(
@@ -420,10 +402,8 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
420402
unchecked {
421403
{
422404
uint requiredFee = getUpdateFee(updateData);
423-
require(
424-
msg.value >= requiredFee,
425-
"insufficient paid fee amount"
426-
);
405+
if (msg.value < requiredFee)
406+
revert PythErrors.InsufficientFee();
427407
}
428408

429409
priceFeeds = new PythStructs.PriceFeed[](priceIds.length);
@@ -482,10 +462,6 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
482462
index,
483463
attestationSize
484464
);
485-
require(
486-
info.publishTime != 0,
487-
"price feed for the given id is not pushed or does not exist"
488-
);
489465

490466
priceFeeds[k].id = priceId;
491467
priceFeeds[k].price.price = info.price;
@@ -513,10 +489,9 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
513489
}
514490

515491
for (uint k = 0; k < priceIds.length; k++) {
516-
require(
517-
priceFeeds[k].id != 0,
518-
"1 or more price feeds are not found in the updateData or they are out of the given time range"
519-
);
492+
if (priceFeeds[k].id == 0) {
493+
revert PythErrors.PriceFeedNotFoundWithinRange();
494+
}
520495
}
521496
}
522497
}
@@ -526,10 +501,7 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
526501
) public view override returns (PythStructs.PriceFeed memory priceFeed) {
527502
// Look up the latest price info for the given ID
528503
PythInternalStructs.PriceInfo memory info = latestPriceInfo(id);
529-
require(
530-
info.publishTime != 0,
531-
"price feed for the given id is not pushed or does not exist"
532-
);
504+
if (info.publishTime == 0) revert PythErrors.PriceFeedNotFound();
533505

534506
priceFeed.id = id;
535507
priceFeed.price.price = info.price;

‎ethereum/contracts/pyth/PythGovernance.sol‎

Lines changed: 30 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import "./PythGovernanceInstructions.sol";
77
import "./PythInternalStructs.sol";
88
import "./PythGetters.sol";
99
import "./PythSetters.sol";
10+
import "@pythnetwork/pyth-sdk-solidity/PythErrors.sol";
1011

1112
import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Upgrade.sol";
1213

@@ -37,19 +38,17 @@ abstract contract PythGovernance is
3738
function verifyGovernanceVM(
3839
bytes memory encodedVM
3940
) internal returns (IWormhole.VM memory parsedVM) {
40-
(IWormhole.VM memory vm, bool valid, string memory reason) = wormhole()
41-
.parseAndVerifyVM(encodedVM);
42-
43-
require(valid, reason);
44-
require(
45-
isValidGovernanceDataSource(vm.emitterChainId, vm.emitterAddress),
46-
"VAA is not coming from the governance data source"
41+
(IWormhole.VM memory vm, bool valid, ) = wormhole().parseAndVerifyVM(
42+
encodedVM
4743
);
4844

49-
require(
50-
vm.sequence > lastExecutedGovernanceSequence(),
51-
"VAA is older than the last executed governance VAA"
52-
);
45+
if (!valid) revert PythErrors.InvalidWormholeVaa();
46+
47+
if (!isValidGovernanceDataSource(vm.emitterChainId, vm.emitterAddress))
48+
revert PythErrors.InvalidGovernanceDataSource();
49+
50+
if (vm.sequence <= lastExecutedGovernanceSequence())
51+
revert PythErrors.OldGovernanceMessage();
5352

5453
setLastExecutedGovernanceSequence(vm.sequence);
5554

@@ -63,16 +62,12 @@ abstract contract PythGovernance is
6362
vm.payload
6463
);
6564

66-
require(
67-
gi.targetChainId == chainId() || gi.targetChainId == 0,
68-
"invalid target chain for this governance instruction"
69-
);
65+
if (gi.targetChainId != chainId() && gi.targetChainId != 0)
66+
revert PythErrors.InvalidGovernanceTarget();
7067

7168
if (gi.action == GovernanceAction.UpgradeContract) {
72-
require(
73-
gi.targetChainId != 0,
74-
"upgrade with chain id 0 is not possible"
75-
);
69+
if (gi.targetChainId == 0)
70+
revert PythErrors.InvalidGovernanceTarget();
7671
upgradeContract(parseUpgradeContractPayload(gi.payload));
7772
} else if (
7873
gi.action == GovernanceAction.AuthorizeGovernanceDataSourceTransfer
@@ -89,11 +84,10 @@ abstract contract PythGovernance is
8984
} else if (
9085
gi.action == GovernanceAction.RequestGovernanceDataSourceTransfer
9186
) {
92-
revert(
93-
"RequestGovernanceDataSourceTransfer can be only part of AuthorizeGovernanceDataSourceTransfer message"
94-
);
87+
// RequestGovernanceDataSourceTransfer can be only part of AuthorizeGovernanceDataSourceTransfer message
88+
revert PythErrors.InvalidGovernanceMessage();
9589
} else {
96-
revert("invalid governance action");
90+
revert PythErrors.InvalidGovernanceMessage();
9791
}
9892
}
9993

@@ -119,33 +113,30 @@ abstract contract PythGovernance is
119113
// If it's valid then its emitter can take over the governance from the current emitter.
120114
// The VAA is checked here to ensure that the new governance data source is valid and can send message
121115
// through wormhole.
122-
(IWormhole.VM memory vm, bool valid, string memory reason) = wormhole()
123-
.parseAndVerifyVM(payload.claimVaa);
124-
require(valid, reason);
116+
(IWormhole.VM memory vm, bool valid, ) = wormhole().parseAndVerifyVM(
117+
payload.claimVaa
118+
);
119+
if (!valid) revert PythErrors.InvalidWormholeVaa();
125120

126121
GovernanceInstruction memory gi = parseGovernanceInstruction(
127122
vm.payload
128123
);
129-
require(
130-
gi.targetChainId == chainId() || gi.targetChainId == 0,
131-
"invalid target chain for this governance instruction"
132-
);
133-
require(
134-
gi.action == GovernanceAction.RequestGovernanceDataSourceTransfer,
135-
"governance data source change inner vaa is not of claim action type"
136-
);
124+
if (gi.targetChainId != chainId() && gi.targetChainId != 0)
125+
revert PythErrors.InvalidGovernanceTarget();
126+
127+
if (gi.action != GovernanceAction.RequestGovernanceDataSourceTransfer)
128+
revert PythErrors.InvalidGovernanceMessage();
137129

138130
RequestGovernanceDataSourceTransferPayload
139131
memory claimPayload = parseRequestGovernanceDataSourceTransferPayload(
140132
gi.payload
141133
);
142134

143135
// Governance data source index is used to prevent replay attacks, so a claimVaa cannot be used twice.
144-
require(
145-
governanceDataSourceIndex() <
146-
claimPayload.governanceDataSourceIndex,
147-
"cannot upgrade to an older governance data source"
148-
);
136+
if (
137+
governanceDataSourceIndex() >=
138+
claimPayload.governanceDataSourceIndex
139+
) revert PythErrors.OldGovernanceMessage();
149140

150141
setGovernanceDataSourceIndex(claimPayload.governanceDataSourceIndex);
151142

0 commit comments

Comments
 (0)