Skip to content

[eth] Use PythErrors everywhere #404

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 29, 2022
Merged

[eth] Use PythErrors everywhere #404

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
75f6474
Remove unnecessary check
ali-bahjati Nov 25, 2022
f28d5ab
Use PythErrors everywhere
ali-bahjati Nov 25, 2022
79edbf9
Fix Pyth.t.sol test bug
ali-bahjati Nov 29, 2022
60b6772
Remove one more error string
ali-bahjati Nov 29, 2022
1f1f8d8
Address review comments
ali-bahjati Nov 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 36 additions & 64 deletions ethereum/contracts/pyth/Pyth.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import "../libraries/external/UnsafeBytesLib.sol";
import "@pythnetwork/pyth-sdk-solidity/AbstractPyth.sol";
import "@pythnetwork/pyth-sdk-solidity/PythStructs.sol";

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

require(
dataSourceEmitterChainIds.length ==
dataSourceEmitterAddresses.length,
"data source arguments should have the same length"
);
if (
dataSourceEmitterChainIds.length !=
dataSourceEmitterAddresses.length
) revert PythErrors.InvalidArgument();

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

require(
!PythGetters.isValidDataSource(ds.chainId, ds.emitterAddress),
"Data source already added"
);
if (PythGetters.isValidDataSource(ds.chainId, ds.emitterAddress))
revert PythErrors.InvalidArgument();

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

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

require(
attestationIndex <= attestationSize,
"INTERNAL: Consumed more than `attestationSize` bytes"
);
if (attestationIndex > attestationSize)
revert PythErrors.InvalidUpdateData();
}
}

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

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

revert(
"no prices in the submitted batch have fresh prices, so this update will have no effect"
);
revert PythErrors.NoFreshUpdate();
}

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

require(
price.publishTime != 0,
"price feed for the given id is not pushed or does not exist"
);
if (price.publishTime == 0) revert PythErrors.PriceFeedNotFound();
}

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

require(
price.publishTime != 0,
"price feed for the given id is not pushed or does not exist"
);
if (price.publishTime == 0) revert PythErrors.PriceFeedNotFound();
}

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

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

uint16 versionMinor = UnsafeBytesLib.toUint16(encoded, index);
// This value is only used as the check below which currently
// never reverts
// uint16 versionMinor = UnsafeBytesLib.toUint16(encoded, index);
index += 2;
require(
versionMinor >= 0,
"invalid version minor, expected 0 or more"
);

// This check is always false as versionMinor is 0, so it is commented.
// in the future that the minor version increases this will have effect.
// if(versionMinor < 0) revert InvalidUpdateData();

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

// Payload ID of 2 required for batch headerBa
require(
payloadId == 2,
"invalid payload ID, expected 2 for BatchPriceAttestation"
);
if (payloadId != 2) revert PythErrors.InvalidUpdateData();
}

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

// Given the message is valid the arithmetic below should not overflow, and
// even if it overflows then the require would fail.
require(
encoded.length == (index + (attestationSize * nAttestations)),
"invalid BatchPriceAttestation size"
);
if (encoded.length != (index + (attestationSize * nAttestations)))
revert PythErrors.InvalidUpdateData();
}
}

Expand All @@ -398,12 +381,11 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
) internal view returns (IWormhole.VM memory vm) {
{
bool valid;
string memory reason;
(vm, valid, reason) = wormhole().parseAndVerifyVM(encodedVm);
require(valid, reason);
(vm, valid, ) = wormhole().parseAndVerifyVM(encodedVm);
if (!valid) revert PythErrors.InvalidWormholeVaa();
}

require(verifyPythVM(vm), "invalid data source chain/emitter ID");
if (!verifyPythVM(vm)) revert PythErrors.InvalidUpdateDataSource();
}

function parsePriceFeedUpdates(
Expand All @@ -420,10 +402,8 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
unchecked {
{
uint requiredFee = getUpdateFee(updateData);
require(
msg.value >= requiredFee,
"insufficient paid fee amount"
);
if (msg.value < requiredFee)
revert PythErrors.InsufficientFee();
}

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

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

for (uint k = 0; k < priceIds.length; k++) {
require(
priceFeeds[k].id != 0,
"1 or more price feeds are not found in the updateData or they are out of the given time range"
);
if (priceFeeds[k].id == 0) {
revert PythErrors.PriceFeedNotFoundWithinRange();
}
}
}
}
Expand All @@ -526,10 +501,7 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
) public view override returns (PythStructs.PriceFeed memory priceFeed) {
// Look up the latest price info for the given ID
PythInternalStructs.PriceInfo memory info = latestPriceInfo(id);
require(
info.publishTime != 0,
"price feed for the given id is not pushed or does not exist"
);
if (info.publishTime == 0) revert PythErrors.PriceFeedNotFound();

priceFeed.id = id;
priceFeed.price.price = info.price;
Expand Down
69 changes: 30 additions & 39 deletions ethereum/contracts/pyth/PythGovernance.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import "./PythGovernanceInstructions.sol";
import "./PythInternalStructs.sol";
import "./PythGetters.sol";
import "./PythSetters.sol";
import "@pythnetwork/pyth-sdk-solidity/PythErrors.sol";

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

Expand Down Expand Up @@ -37,19 +38,17 @@ abstract contract PythGovernance is
function verifyGovernanceVM(
bytes memory encodedVM
) internal returns (IWormhole.VM memory parsedVM) {
(IWormhole.VM memory vm, bool valid, string memory reason) = wormhole()
.parseAndVerifyVM(encodedVM);

require(valid, reason);
require(
isValidGovernanceDataSource(vm.emitterChainId, vm.emitterAddress),
"VAA is not coming from the governance data source"
(IWormhole.VM memory vm, bool valid, ) = wormhole().parseAndVerifyVM(
encodedVM
);

require(
vm.sequence > lastExecutedGovernanceSequence(),
"VAA is older than the last executed governance VAA"
);
if (!valid) revert PythErrors.InvalidWormholeVaa();

if (!isValidGovernanceDataSource(vm.emitterChainId, vm.emitterAddress))
revert PythErrors.InvalidGovernanceDataSource();

if (vm.sequence <= lastExecutedGovernanceSequence())
revert PythErrors.OldGovernanceMessage();

setLastExecutedGovernanceSequence(vm.sequence);

Expand All @@ -63,16 +62,12 @@ abstract contract PythGovernance is
vm.payload
);

require(
gi.targetChainId == chainId() || gi.targetChainId == 0,
"invalid target chain for this governance instruction"
);
if (gi.targetChainId != chainId() && gi.targetChainId != 0)
revert PythErrors.InvalidGovernanceTarget();

if (gi.action == GovernanceAction.UpgradeContract) {
require(
gi.targetChainId != 0,
"upgrade with chain id 0 is not possible"
);
if (gi.targetChainId == 0)
revert PythErrors.InvalidGovernanceTarget();
upgradeContract(parseUpgradeContractPayload(gi.payload));
} else if (
gi.action == GovernanceAction.AuthorizeGovernanceDataSourceTransfer
Expand All @@ -89,11 +84,10 @@ abstract contract PythGovernance is
} else if (
gi.action == GovernanceAction.RequestGovernanceDataSourceTransfer
) {
revert(
"RequestGovernanceDataSourceTransfer can be only part of AuthorizeGovernanceDataSourceTransfer message"
);
// RequestGovernanceDataSourceTransfer can be only part of AuthorizeGovernanceDataSourceTransfer message
revert PythErrors.InvalidGovernanceMessage();
} else {
revert("invalid governance action");
revert PythErrors.InvalidGovernanceMessage();
}
}

Expand All @@ -119,33 +113,30 @@ abstract contract PythGovernance is
// 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);
(IWormhole.VM memory vm, bool valid, ) = wormhole().parseAndVerifyVM(
payload.claimVaa
);
if (!valid) revert PythErrors.InvalidWormholeVaa();

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"
);
if (gi.targetChainId != chainId() && gi.targetChainId != 0)
revert PythErrors.InvalidGovernanceTarget();

if (gi.action != GovernanceAction.RequestGovernanceDataSourceTransfer)
revert PythErrors.InvalidGovernanceMessage();

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"
);
if (
governanceDataSourceIndex() >=
claimPayload.governanceDataSourceIndex
) revert PythErrors.OldGovernanceMessage();

setGovernanceDataSourceIndex(claimPayload.governanceDataSourceIndex);

Expand Down
Loading