fix(pulse): Miscellaneous contract fixes #2454

jayantk · 2025-03-08T22:46:52Z

Summary

I was looking through the Pulse contract and noticed some issues:

There's a security issue with the IPulseConsumer interface as anyone can mock data and pass it as the legitimate response to the request
publishTimes are unix timestamps so should be uint64
The provider fee / backup mechanism doesn't work properly because it doesn't account for pyth fees, and it also doesn't give any incentive to backup providers.

How has this been tested?

Current tests cover my changes
Added new tests
Manually tested the code

vercel · 2025-03-08T22:46:57Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
api-reference	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 8, 2025 10:55pm
proposals	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 8, 2025 10:55pm
staking	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 8, 2025 10:55pm

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
component-library	⬜️ Ignored (Inspect)	Visit Preview	Mar 8, 2025 10:55pm
entropy-debugger	⬜️ Ignored (Inspect)	Visit Preview	Mar 8, 2025 10:55pm
insights	⬜️ Ignored (Inspect)	Visit Preview	Mar 8, 2025 10:55pm

jayantk · 2025-03-08T22:47:58Z

target_chains/ethereum/contracts/contracts/pulse/IPulse.sol

+
+    // 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.


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.

jayantk · 2025-03-08T22:52:53Z

target_chains/ethereum/contracts/contracts/pulse/IPulse.sol

    ) external view returns (uint128 feeAmount);

-    function getAccruedFees() external view returns (uint128 accruedFeesInWei);
+    function getAccruedPythFees()


(renamed this for clarity)

jayantk · 2025-03-08T22:54:12Z

target_chains/ethereum/contracts/contracts/pulse/Pulse.sol

@@ -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);


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

jayantk · 2025-03-08T22:55:11Z

target_chains/ethereum/contracts/contracts/pulse/Pulse.sol

-                SafeCast.toUint64(req.publishTime),
-                SafeCast.toUint64(req.publishTime)
-            );
+        // TODO: should this use parsePriceFeedUpdatesUnique? also, do we need to add 1 to maxPublishTime?


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)

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

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

jayantk · 2025-03-08T22:56:02Z

target_chains/ethereum/contracts/contracts/pulse/Pulse.sol

@@ -111,7 +112,7 @@ abstract contract Pulse is IPulse, PulseState {
            block.timestamp < req.publishTime + _state.exclusivityPeriodSeconds
        ) {
            require(
-                msg.sender == req.provider,
+                providerToCredit == req.provider,


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.

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

jayantk · 2025-03-08T22:59:38Z

target_chains/ethereum/contracts/contracts/pulse/Pulse.sol

+        require(
+            msg.sender == provider ||
+                msg.sender == _state.providers[provider].feeManager,
+            "Only provider or fee manager can invoke this method"


the fee manager should be allowed to set fees

ali-behjati · 2025-03-11T12:41:56Z

target_chains/ethereum/contracts/contracts/pulse/IPulse.sol

@@ -30,7 +52,8 @@ interface IPulse is PulseEvents {
     *      the fee estimation unreliable.
     */
    function requestPriceUpdatesWithCallback(
-        uint256 publishTime,
+        address provider,


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? ...)

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.

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

ali-behjati

This PR had a lot of small high context changes. I generally recommend separating bug fixes vs other changes and specially the API changes.

ali-behjati · 2025-03-11T18:46:40Z

target_chains/ethereum/contracts/contracts/pulse/IPulse.sol

@@ -30,7 +52,8 @@ interface IPulse is PulseEvents {
     *      the fee estimation unreliable.
     */
    function requestPriceUpdatesWithCallback(
-        uint256 publishTime,
+        address provider,


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

ali-behjati · 2025-03-11T18:49:04Z

target_chains/ethereum/contracts/contracts/pulse/PulseState.sol

+        // TODO: this is going to absolutely explode gas costs. Need to do something smarter here.
+        // possible solution is to hash the price ids and store the hash instead.
+        // The ids themselves can be retrieved from the event.


We decided to move away from event because that adds a lot of complexity. We can come back to it if the cost is substantial (and the costs generally are getting lower on ETH which is the only chain that has high costs on these things)

ali-behjati · 2025-03-11T18:51:27Z

target_chains/ethereum/contracts/contracts/pulse/Pulse.sol

+        // 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.


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

ali-behjati · 2025-03-11T18:54:26Z

target_chains/ethereum/contracts/contracts/pulse/Pulse.sol

+        // 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.


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

ali-behjati · 2025-03-11T18:54:44Z

target_chains/ethereum/contracts/contracts/pulse/Pulse.sol

-                SafeCast.toUint64(req.publishTime),
-                SafeCast.toUint64(req.publishTime)
-            );
+        // TODO: should this use parsePriceFeedUpdatesUnique? also, do we need to add 1 to maxPublishTime?


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

ali-behjati · 2025-03-11T19:00:58Z

target_chains/ethereum/contracts/contracts/pulse/Pulse.sol

        _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.


good call no it's not needed.

ali-behjati · 2025-03-11T19:01:20Z

target_chains/ethereum/contracts/contracts/pulse/Pulse.sol

        require(
            _state.providers[provider].isRegistered,
            "Provider not registered"
        );

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


remove the wrong comment then?

ali-behjati · 2025-03-11T19:04:26Z

target_chains/ethereum/contracts/contracts/pulse/Pulse.sol

+            // Safe cast because this is a unix timestamp in seconds.
+            publishTimes[i] = SafeCast.toUint64(
+                priceFeeds[i].price.publishTime
+            );


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

ali-behjati · 2025-03-11T19:06:48Z

target_chains/ethereum/contracts/contracts/pulse/Pulse.sol

@@ -111,7 +112,7 @@ abstract contract Pulse is IPulse, PulseState {
            block.timestamp < req.publishTime + _state.exclusivityPeriodSeconds
        ) {
            require(
-                msg.sender == req.provider,
+                providerToCredit == req.provider,


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

ali-behjati · 2025-03-11T19:08:02Z

target_chains/ethereum/contracts/contracts/pulse/Pulse.sol


        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


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

jayantk added 5 commits March 7, 2025 10:31

fix stuff

97695be

more stuff

dd205a0

pulsse fixes

8d9f689

fix

dfdf1f7

fix tests

bd6fd22

jayantk changed the title ~~Pulse~~ fix(pulse): Miscellaneous contract fixes Mar 8, 2025

vercel bot deployed to Preview – api-reference March 8, 2025 22:47 View deployment

vercel bot deployed to Preview – proposals March 8, 2025 22:47 View deployment

jayantk commented Mar 8, 2025

View reviewed changes

gr

b7bc50c

jayantk commented Mar 8, 2025

View reviewed changes

vercel bot deployed to Preview – api-reference March 8, 2025 22:54 View deployment

vercel bot deployed to Preview – staking March 8, 2025 22:54 View deployment

jayantk commented Mar 8, 2025

View reviewed changes

vercel bot deployed to Preview – proposals March 8, 2025 22:55 View deployment

jayantk commented Mar 8, 2025

View reviewed changes

ali-behjati reviewed Mar 11, 2025

View reviewed changes

cctdaniel approved these changes Mar 11, 2025

View reviewed changes

jayantk merged commit 92a1737 into main Mar 11, 2025
11 checks passed

jayantk deleted the pulse branch March 11, 2025 18:42

ali-behjati reviewed Mar 11, 2025

View reviewed changes

		// 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.

fix(pulse): Miscellaneous contract fixes #2454

fix(pulse): Miscellaneous contract fixes #2454

Uh oh!

Conversation

jayantk commented Mar 8, 2025

Summary

How has this been tested?

Uh oh!

vercel bot commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayantk Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayantk Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ali-behjati left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vercel bot commented Mar 8, 2025 •

edited

Loading

jayantk Mar 8, 2025 •

edited

Loading

jayantk Mar 8, 2025 •

edited

Loading