Rewards Tree Spec v10

Governance
21

So to put this on the forum as well:
RPIP-31 differentiates between claiming RPL rewards and claiming and staking:

  • for claim only (the section you referenced), it indeed seems to imply that any of the node’s primary withdrawal address, the current RPL withdrawal address, or the node’s address should be able to call it
  • for claim and stake, it specifies that only the RPL withdrawal address should be able to (if one is set)

The contract implementation does a check for this in the _claimAndStake() function and implements the claim() function by calling _claimAndStake() under the hood, so uses one restriction for both.

Interestingly, the Sigma Prime Audit partially noticed this discrepancy in RPH-11. Originally the contracts allowed all three in both cases. Unfortunately they didn’t quite put together that there should be a differentiation between claim() and claimAndStake() and so the fix ended up being restricting to RPL withdrawal address only in both cases.

So I agree that contracts currently don’t follow RPIP-31, but disagree on the specifics. There should be different checks for claiming and claiming and staking according to the RPIP.

2 Likes
22

I want to highlight another issue I see with the implementation of RPIP-31 in Houston.

Vanilla Houston (without v10)

To reiterate, if a RPL withdrawal address is set and without a reward tree change only that RPL reward address is able to claim both RPL and smoothing pool rewards for a node. The implementation of the RPL claim executes a call to the primary withdrawal address to send the smoothing pool ETH there. This opens the door to what I view as a griefing attack that goes against what RPIP-31 specifies and should be considered a bug:
The controller of the primary withdrawal address can set it to an arbitrary address at any point. If they set it to a smart contract that doesn’t define a payable fallback, the call to the withdrawal address will revert and therefore the entire claim (including of RPL) will revert as well.
The primary controller has the ability to block the controller of RPL from claiming rewards.

Houston with v10

A side effect of v10 discussed here is that it limits (but doesn’t eliminate) this griefing attack. If the primary is set to an address different from the node wallet, RPL and ETH claims are separated and the griefing is no longer possible.
If the node wallet is used as the primary, the primary controller still has the ability to block the controller of RPL from claiming rewards, because RPL and ETH are still claimed together.
Note that it is possible to switch the primary back to the node wallet if a different primary was set.
By confirming that a node wallet is not a smart contract initially, one can ensure that this attack is impossible.

Is this a bug or not?

The Medium post about Houston introduces the RPL withdrawal address like this:

With this new addition, you can set a withdrawal address for your ETH and a new one for your RPL if you wish. The RPL withdrawal address if set will be able to trigger and claim RPL from inflation rewards and will have no effect on ETH consensus rewards or anything related to ETH.
This creates some interesting opportunities where RPL can be trustlessly supplied by an entity to a node operator that does not wish to have exposure to RPL. That entity can then claim RPL rewards for putting up the required insurance collateral for the node.

The claiming process is presented as trustless, but as explained above the RPL controller needs to trust the primary controller to not block the claim in vanilla Houston and needs to verify that it is not possible in v10 Houston.

RPIP-31 defines claiming of RPL rewards as clearly in the domain of the RPL controller and independent of the primary controller:

As the controller of the RPL for a node, I MUST be able to trigger a claim of RPL rewards

In the Security Considerations section the following limitation is highlighted:

In a situation were the node operator is different to the RPL owner, the RPL owner must rely on the node operator to exit minipools to unlock their RPL stake.

But there is no mention of a reliance on the node operator for claiming rewards, implying that no such thing exists.

In summary, this clearly is unintended behavior and should be viewed as a bug.

Recommendations

  • Deploying vanilla Houston without fixing this bug and without v10 shouldn’t be done.
  • If Houston is deployed as is and v10 is implemented, we should clearly communicate the risk. This should include editing the medium post and RPIP-31. The latter would require a pDAO vote in my opinion.
  • If additional time is spent on fixing RPIP-31 implementation in a future upgrade, this griefing vector should be kept in mind and avoided.
3 Likes
23

Hi all, I wanted to chime in about what NodeSet needs from Houston for Constellation and how this relates to rewards tree v10 discussions.

First of all, it is critical that the Constellation protocol has a way to claim smoothing pool and RPL rewards with an O(1) algorithm – i.e. a single transaction instead of looping through all the nodes and submitting a claim transaction for each one. There are two ways to achieve this:

  1. Constellation uses a single “meganode” contract, and all NS operators deploy minipool contracts using this node. This would be fairly complex, because it involves managing “sub nodes” that are internal to Constellation while still being transparent to Rocket Pool. More importantly, it opens up major issues for operations that are node-specific such as voting on Protocol DAO proposals. The meganode approach means there could only be one vote that represents all of NS, and whoever controls Constellation’s upgrades (i.e. the administrator) would have unilateral control over its voting power. That’s not what anybody wants, and it’s enough of a reason on its own to reject this design path.

  2. Every NS node operator manages their own Rocket Pool node. They deploy their own node contract, which becomes a registered Rocket Pool node operator, but as part of that, they set a Constellation contract as the ETH and RPL withdrawal address. This solves all of the problems with strategy 1, and it lets nodes vote individually on Protocol DAO proposals however they like. It’s in line with NodeSet’s mission of empowering individual node operators, but importantly, it doesn’t work under the current rewards claiming rules because it requires a claim transaction for every node.

Obviously, we would prefer strategy 2, so several weeks ago we spoke to the RP team about it in detail. After some back and forth, the disparity raised in RPIP-53 between the intended and actual behavior for the Houston contracts was discovered. As part of that conversation though, we found that it’s technically possible to enable this rewards consolidation without changing the contracts. It would just require changing the way the rewards tree was generated. Due to the code freeze on the contracts, both teams came to the conclusion that this approach was the most realistic path forward in the short term. Thus, the rewards spec v10 was born.

Since the RP team has (reasonably IMO) told us that changes to the contracts are not appropriate at this stage, and since we are already in the testing/polishing phase of Constellation’s development using frozen Houston contracts anyway, we need the v10 rewards spec. Therefore, we encourage the adoption of RPIP-53 for now and recommend exploring refinement of the Rocket Pool contracts in Saturn.

tl;dr: Constellation needs either RPIP-53 or a contract change in Houston. Even if contracts are changed now, it would delay us significantly, as well, so we support RPIP-53.

24

If you recommend exploring refinement in Saturn why do you not vote for option 2 in the poll? I guess because you care a lot more about v10 at all than needing to improve things eventually? The poll doesn’t really allow us to capture preferences that well so I want to hear more on why you wouldn’t push to improve in Saturn, if that’s true.

25

I understand why O(1) is useful for your application. It is not, however, part of RPIP-31 at all. RPIP-31 talks about who can initiate RPL claims, but it does not talk about aggregating them or not in any way. (Eg, if a withdrawal address was used for 5 nodes and needed to make one claim for each, that strategy could fulfill the spec). This is also true for O(1) smoothing pool claims – ETH is mentioned exactly once in RPIP-31 to say that claiming RPL will also distribute smoothing pool ETH for that NO. Again - no mention one way or the other about aggregation. I think it’s important to clarify that the thing you’re describing as critical to Nodeset is not something that the pDAO has at any time voted in. Again, I totally get why it’s useful to your application, but from a governance perspective this is not something that has been committed to in any way.

Fwiw, I think my preference remains v10 simply as being most pragmatic, though item #1 in Houston Specification Discrepancies does give me some pause.

2 Likes
26

Consolidation of claims was definitely not part of RPIP-31, so I’m surprised to hear it’s critical for nodeset.

So far this discussion has focused on v10 as a partial fix of buggy houston contracts, with claim consolidation being a side effect. I don’t think we should sneak in critical protocol changes this way. Have we looked at unintended second order effects? What does consolidation do to sybil resistance of governance?

1 Like
27

Hmm, so what I’m reading here is that this information about what NodeSet “needs” was discussed early on but you only clearly state it here two weeks after the post has been made?

Honestly, I was excited about NodeSet and, as a small NO, am considering hopping on board. But it demanding stuff from Rocket Pool is not really cool. It should be something that just works on top of Rocket Pool and doesn’t make demands.

And if you do demand things, they should clearly state this early on. If you already discussed this, why did it take 2 weeks to communicate this?

Your review of the tokenomic changes also came 2 weeks after the discussion had been started. Yes, I appreciate that it was a big report. But it’s still 2 weeks that people have been discussing the proposed changes without knowing that this would impact NodeSet quite a bit.

In my oppinion, you need to either stop demanding things from Rocket Pool and just roll with the punches or be a central part of every discussion so proposals that affect you already have your input/view when they get proposed. The current way where you come “out of the woodwork” a few weeks into the discussion and heavily influence it by saying that you “need” it in a certain way is not feasible. It makes conversations feel useless because we always have to wonder “but what about NodeSet?”.

So please, NodeSet either must become more integral to the discussions here, or just stop throwing rocks onto Rocket Pool’s roads.

28

I do think it’s a good idea to improve it in Saturn, but our support for v10 is not predicated upon this.

29

The team recently announced that they were going to fix the contract deviations from RPIP-31 prior to launching Houston. As such, this RPIP (and consequently this proposal for rewards v10) is now OBE. I have marked it as withdrawn and we should now consider the whole thing closed.

Thanks to everyone for your help with this and investigation into the issues it’d raise. I think Houston’s in a much better situation now.

2 Likes