Skip to content

fix(pulse): Miscellaneous contract fixes #2454

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 6 commits into from
Mar 11, 2025
Merged

fix(pulse): Miscellaneous contract fixes #2454

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97695be
fix stuff
jayantk Mar 7, 2025
dd205a0
more stuff
jayantk Mar 7, 2025
8d9f689
pulsse fixes
jayantk Mar 8, 2025
dfdf1f7
fix
jayantk Mar 8, 2025
bd6fd22
fix tests
jayantk Mar 8, 2025
b7bc50c
gr
jayantk Mar 8, 2025
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
51 changes: 45 additions & 6 deletions target_chains/ethereum/contracts/contracts/pulse/IPulse.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,40 @@ import "@pythnetwork/pyth-sdk-solidity/IPyth.sol";
import "./PulseEvents.sol";
import "./PulseState.sol";

interface IPulseConsumer {
abstract contract IPulseConsumer {
// This method is called by Pulse to provide the price updates to the consumer.
// It asserts that the msg.sender is the Pulse contract. It is not meant to be
// overridden by the consumer.
function _pulseCallback(
uint64 sequenceNumber,
PythStructs.PriceFeed[] memory priceFeeds
) external {
address pulse = getPulse();
require(pulse != address(0), "Pulse address not set");
require(msg.sender == pulse, "Only Pulse can call this function");

pulseCallback(sequenceNumber, priceFeeds);
}

// getPulse returns the Pulse contract address. The method is being used to check that the
// callback is indeed from the Pulse contract. The consumer is expected to implement this method.
function getPulse() internal view virtual returns (address);

// This method is expected to be implemented by the consumer to handle the price updates.
// It will be called by _pulseCallback after _pulseCallback ensures that the call is
// indeed from Pulse contract.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that if this is an external method, then anyone could have called this function with a sequenceNumber and a list of PriceFeeds (which are already parsed, and thus mockable).

Fix here ensures that only the Pulse contract can invoke the callback.

function pulseCallback(
uint64 sequenceNumber,
PythStructs.PriceFeed[] memory priceFeeds
) external;
) internal virtual;
}

interface IPulse is PulseEvents {
// Core functions
/**
* @notice Requests price updates with a callback
* @dev The msg.value must be equal to getFee(callbackGasLimit)
* @param provider The provider to fulfill the request
* @param callbackGasLimit The amount of gas allocated for the callback execution
* @param publishTime The minimum publish time for price updates, it should be less than or equal to block.timestamp + 60
* @param priceIds The price feed IDs to update. Maximum 10 price feeds per request.
Expand All @@ -30,7 +52,8 @@ interface IPulse is PulseEvents {
* the fee estimation unreliable.
*/
function requestPriceUpdatesWithCallback(
uint256 publishTime,
address provider,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like people choosing their providers. It's an extra hop for integrators to think about (what are providers? how they are different? how should i choose? ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah agreed. I think the thing to do is to add a wrapper method that chooses for users. I'm not exactly sure what the right way to choose is yet though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well if people are gonna use the wrapper it shouldn't be public right? (I recommend finalizing what people are gonna use)

uint64 publishTime,
bytes32[] calldata priceIds,
uint256 callbackGasLimit
) external payable returns (uint64 sequenceNumber);
Expand All @@ -39,11 +62,13 @@ interface IPulse is PulseEvents {
* @notice Executes the callback for a price update request
* @dev Requires 1.5x the callback gas limit to account for cross-contract call overhead
* For example, if callbackGasLimit is 1M, the transaction needs at least 1.5M gas + some gas for some other operations in the function before the callback
* @param providerToCredit The provider to credit for fulfilling the request. This may not be the provider that submitted the request (if the exclusivity period has elapsed).
* @param sequenceNumber The sequence number of the request
* @param updateData The raw price update data from Pyth
* @param priceIds The price feed IDs to update, must match the request
*/
function executeCallback(
address providerToCredit,
uint64 sequenceNumber,
bytes[] calldata updateData,
bytes32[] calldata priceIds
Expand All @@ -59,12 +84,17 @@ interface IPulse is PulseEvents {

/**
* @notice Calculates the total fee required for a price update request
* FIXME: is this comment right?
* @dev Total fee = base Pyth protocol fee + gas costs for callback
* @param provider The provider to fulfill the request
* @param callbackGasLimit The amount of gas allocated for callback execution
* @param priceIds The price feed IDs to update.
* @return feeAmount The total fee in wei that must be provided as msg.value
*/
function getFee(
uint256 callbackGasLimit
address provider,
uint256 callbackGasLimit,
bytes32[] calldata priceIds
) external view returns (uint128 feeAmount);

function getAccruedFees() external view returns (uint128 accruedFeesInWei);
Expand All @@ -83,9 +113,18 @@ interface IPulse is PulseEvents {

function withdrawAsFeeManager(address provider, uint128 amount) external;

function registerProvider(uint128 feeInWei) external;
function registerProvider(
uint128 baseFeeInWei,
uint128 feePerFeedInWei,
uint128 feePerGasInWei
) external;

function setProviderFee(uint128 newFeeInWei) external;
function setProviderFee(
address provider,
uint128 newBaseFeeInWei,
uint128 newFeePerFeedInWei,
uint128 newFeePerGasInWei
) external;

function getProviderInfo(
address provider
Expand Down
143 changes: 101 additions & 42 deletions target_chains/ethereum/contracts/contracts/pulse/Pulse.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,19 @@ abstract contract Pulse is IPulse, PulseState {
}
}

// TODO: there can be a separate wrapper function that defaults the provider (or uses the cheapest or something).
function requestPriceUpdatesWithCallback(
uint256 publishTime,
address provider,
uint64 publishTime,
bytes32[] calldata priceIds,
uint256 callbackGasLimit
) external payable override returns (uint64 requestSequenceNumber) {
address provider = _state.defaultProvider;
require(
_state.providers[provider].isRegistered,
"Provider not registered"
);

// FIXME: this comment is wrong. (we're not using tx.gasprice)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the wrong comment then?

// NOTE: The 60-second future limit on publishTime prevents a DoS vector where
// attackers could submit many low-fee requests for far-future updates when gas prices
// are low, forcing executors to fulfill them later when gas prices might be much higher.
Expand All @@ -75,7 +77,7 @@ abstract contract Pulse is IPulse, PulseState {
}
requestSequenceNumber = _state.currentSequenceNumber++;

uint128 requiredFee = getFee(callbackGasLimit);
uint128 requiredFee = getFee(provider, callbackGasLimit, priceIds);
if (msg.value < requiredFee) revert InsufficientFee();

Request storage req = allocRequest(requestSequenceNumber);
Expand All @@ -85,21 +87,20 @@ abstract contract Pulse is IPulse, PulseState {
req.requester = msg.sender;
req.numPriceIds = uint8(priceIds.length);
req.provider = provider;
req.fee = SafeCast.toUint128(msg.value - _state.pythFeeInWei);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Store the fee in the request so we can credit the provider who actually fulfilled the request in executeCallback


// Copy price IDs to storage
for (uint8 i = 0; i < priceIds.length; i++) {
req.priceIds[i] = priceIds[i];
}

_state.providers[provider].accruedFeesInWei += SafeCast.toUint128(
msg.value - _state.pythFeeInWei
);
_state.accruedFeesInWei += _state.pythFeeInWei;

emit PriceUpdateRequested(req, priceIds);
}

// TODO: does this need to be payable? Any cost paid to Pyth could be taken out of the provider's accrued fees.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call no it's not needed.

function executeCallback(
address providerToCredit,
uint64 sequenceNumber,
bytes[] calldata updateData,
bytes32[] calldata priceIds
Expand All @@ -111,7 +112,7 @@ abstract contract Pulse is IPulse, PulseState {
block.timestamp < req.publishTime + _state.exclusivityPeriodSeconds
) {
require(
msg.sender == req.provider,
providerToCredit == req.provider,
Copy link
Contributor Author

@jayantk jayantk Mar 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the provider key is a cold key that shouldn't be used in hot actions. (This is why we have the feeManager as the hot key that has limited permissions -- if it is compromised, the provider can rotate it without too much damage)

Instead, make this method permissionless. Only the requested provider is motivated to call it during the exclusivity period as that's the only person who can earn the fees for the request.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add a check to make sure the provider is registered?

"Only assigned provider during exclusivity period"
);
}
Expand All @@ -127,19 +128,41 @@ abstract contract Pulse is IPulse, PulseState {
}
}

// Parse price feeds first to measure gas usage
PythStructs.PriceFeed[] memory priceFeeds = IPyth(_state.pyth)
.parsePriceFeedUpdates(
updateData,
priceIds,
SafeCast.toUint64(req.publishTime),
SafeCast.toUint64(req.publishTime)
);
// TODO: should this use parsePriceFeedUpdatesUnique? also, do we need to add 1 to maxPublishTime?
Copy link
Contributor Author

@jayantk jayantk Mar 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the tests mock the parsePriceFeedUpdates call, so we don't actually know if it's being invoked correctly. (it was definitely missing the value parameter)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using parsePriceFeedUpdatesUnique here is a good idea due to its deterministic behavior

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I echo what Daniel says here. No you don't need to add 1 to it.

IPyth pyth = IPyth(_state.pyth);
uint256 pythFee = pyth.getUpdateFee(updateData);
PythStructs.PriceFeed[] memory priceFeeds = pyth.parsePriceFeedUpdates{
value: pythFee
}(
updateData,
priceIds,
SafeCast.toUint64(req.publishTime),
SafeCast.toUint64(req.publishTime)
);

// TODO: if this effect occurs here, we need to guarantee that executeCallback can never revert.
// If executeCallback can revert, then funds can be permanently locked in the contract.
Comment on lines +143 to +144
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong with just considering them as Pyth fee? I think we mention the risks in the interface.

// TODO: there also needs to be some penalty mechanism in case the expected provider doesn't execute the callback.
// This should take funds from the expected provider and give to providerToCredit. The penalty should probably scale
// with time in order to ensure that the callback eventually gets executed.
// (There may be exploits with ^ though if the consumer contract is malicious ?)
Comment on lines +145 to +148
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best to worry about it later and this comment can be removed imo. I doubt penalizing downtime is great; giving credits and using that for assignment is a better option for the future.

_state.providers[providerToCredit].accruedFeesInWei += SafeCast
.toUint128((req.fee + msg.value) - pythFee);

clearRequest(sequenceNumber);

// TODO: I'm pretty sure this is going to use a lot of gas because it's doing a storage lookup for each sequence number.
// a better solution would be a doubly-linked list of active requests.
// After successful callback, update firstUnfulfilledSeq if needed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah true. let's do some gas benchmarking later.

while (
_state.firstUnfulfilledSeq < _state.currentSequenceNumber &&
!isActive(findRequest(_state.firstUnfulfilledSeq))
) {
_state.firstUnfulfilledSeq++;
}

try
IPulseConsumer(req.requester).pulseCallback{
IPulseConsumer(req.requester)._pulseCallback{
gas: req.callbackGasLimit
}(sequenceNumber, priceFeeds)
{
Expand All @@ -149,7 +172,7 @@ abstract contract Pulse is IPulse, PulseState {
// Explicit revert/require
emit PriceUpdateCallbackFailed(
sequenceNumber,
msg.sender,
providerToCredit,
priceIds,
req.requester,
reason
Expand All @@ -158,20 +181,12 @@ abstract contract Pulse is IPulse, PulseState {
// Out of gas or other low-level errors
emit PriceUpdateCallbackFailed(
sequenceNumber,
msg.sender,
providerToCredit,
priceIds,
req.requester,
"low-level error (possibly out of gas)"
);
}

// After successful callback, update firstUnfulfilledSeq if needed
while (
_state.firstUnfulfilledSeq < _state.currentSequenceNumber &&
!isActive(findRequest(_state.firstUnfulfilledSeq))
) {
_state.firstUnfulfilledSeq++;
}
}

function emitPriceUpdate(
Expand All @@ -182,13 +197,16 @@ abstract contract Pulse is IPulse, PulseState {
int64[] memory prices = new int64[](priceFeeds.length);
uint64[] memory conf = new uint64[](priceFeeds.length);
int32[] memory expos = new int32[](priceFeeds.length);
uint256[] memory publishTimes = new uint256[](priceFeeds.length);
uint64[] memory publishTimes = new uint64[](priceFeeds.length);

for (uint i = 0; i < priceFeeds.length; i++) {
prices[i] = priceFeeds[i].price.price;
conf[i] = priceFeeds[i].price.conf;
expos[i] = priceFeeds[i].price.expo;
publishTimes[i] = priceFeeds[i].price.publishTime;
// Safe cast because this is a unix timestamp in seconds.
publishTimes[i] = SafeCast.toUint64(
priceFeeds[i].price.publishTime
);
Comment on lines +206 to +209
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure it helps with costs, afaik event arguments will end up as u256 regardless.

}

emit PriceUpdateExecuted(
Expand All @@ -203,14 +221,25 @@ abstract contract Pulse is IPulse, PulseState {
}

function getFee(
uint256 callbackGasLimit
address provider,
uint256 callbackGasLimit,
bytes32[] calldata priceIds
) public view override returns (uint128 feeAmount) {
uint128 baseFee = _state.pythFeeInWei; // Fixed fee to Pyth
uint128 providerFeeInWei = _state
.providers[_state.defaultProvider]
.feeInWei; // Provider's per-gas rate
// Note: The provider needs to set its fees to include the fee charged by the Pyth contract.
// Ideally, we would be able to automatically compute the pyth fees from the priceIds, but the
// fee computation on IPyth assumes it has the full updated data.
Comment on lines +229 to +231
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't you at least set it a constant for the actual contract? Now the provider fees are getting complicated.

uint128 providerBaseFee = _state.providers[provider].baseFeeInWei;
uint128 providerFeedFee = SafeCast.toUint128(
priceIds.length * _state.providers[provider].feePerFeedInWei
);
uint128 providerFeeInWei = _state.providers[provider].feePerGasInWei; // Provider's per-gas rate
uint256 gasFee = callbackGasLimit * providerFeeInWei; // Total provider fee based on gas
feeAmount = baseFee + SafeCast.toUint128(gasFee); // Total fee user needs to pay
feeAmount =
baseFee +
providerBaseFee +
providerFeedFee +
SafeCast.toUint128(gasFee); // Total fee user needs to pay
}

function getPythFeeInWei()
Expand Down Expand Up @@ -244,6 +273,7 @@ abstract contract Pulse is IPulse, PulseState {
shortHash = uint8(hash[0] & NUM_REQUESTS_MASK);
}

// TODO: move out governance functions into a separate PulseGovernance contract
function withdrawFees(uint128 amount) external override {
require(msg.sender == _state.admin, "Only admin can withdraw fees");
require(_state.accruedFeesInWei >= amount, "Insufficient balance");
Expand Down Expand Up @@ -336,22 +366,51 @@ abstract contract Pulse is IPulse, PulseState {
emit FeesWithdrawn(msg.sender, amount);
}

function registerProvider(uint128 feeInWei) external override {
function registerProvider(
uint128 baseFeeInWei,
uint128 feePerFeedInWei,
uint128 feePerGasInWei
) external override {
ProviderInfo storage provider = _state.providers[msg.sender];
require(!provider.isRegistered, "Provider already registered");
provider.feeInWei = feeInWei;
provider.baseFeeInWei = baseFeeInWei;
provider.feePerFeedInWei = feePerFeedInWei;
provider.feePerGasInWei = feePerGasInWei;
provider.isRegistered = true;
emit ProviderRegistered(msg.sender, feeInWei);
emit ProviderRegistered(msg.sender, feePerGasInWei);
}

function setProviderFee(uint128 newFeeInWei) external override {
function setProviderFee(
address provider,
uint128 newBaseFeeInWei,
uint128 newFeePerFeedInWei,
uint128 newFeePerGasInWei
) external override {
require(
_state.providers[msg.sender].isRegistered,
_state.providers[provider].isRegistered,
"Provider not registered"
);
uint128 oldFee = _state.providers[msg.sender].feeInWei;
_state.providers[msg.sender].feeInWei = newFeeInWei;
emit ProviderFeeUpdated(msg.sender, oldFee, newFeeInWei);
require(
msg.sender == provider ||
msg.sender == _state.providers[provider].feeManager,
"Only provider or fee manager can invoke this method"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fee manager should be allowed to set fees

);

uint128 oldBaseFee = _state.providers[provider].baseFeeInWei;
uint128 oldFeePerFeed = _state.providers[provider].feePerFeedInWei;
uint128 oldFeePerGas = _state.providers[provider].feePerGasInWei;
_state.providers[provider].baseFeeInWei = newBaseFeeInWei;
_state.providers[provider].feePerFeedInWei = newFeePerFeedInWei;
_state.providers[provider].feePerGasInWei = newFeePerGasInWei;
emit ProviderFeeUpdated(
provider,
oldBaseFee,
oldFeePerFeed,
oldFeePerGas,
newBaseFeeInWei,
newFeePerFeedInWei,
newFeePerGasInWei
);
}

function getProviderInfo(
Expand Down
1 change: 1 addition & 0 deletions target_chains/ethereum/contracts/contracts/pulse/PulseErrors.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pragma solidity ^0.8.0;

error NoSuchProvider();
error NoSuchRequest();
// TODO: add expected / provided values
error InsufficientFee();
error Unauthorized();
error InvalidCallbackGas();
Expand Down
Loading
Loading