fix: ensure getRewards and takeRewards return 0 for closed allos (L-11) #1149
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fix: ensure getRewards and takeRewards return 0 for closed allos (L-11)
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
tmigone
force-pushed
the
horizon-oz2/l10-unsafe-encoding
branch
from
0d9825c to
d6783a9
Compare
tmigone
force-pushed
the
horizon-oz2/l11-rewards-closed-allos
branch
from
caa7fa9 to
93a3838
Compare
tmigone
force-pushed
the
horizon-oz2/l10-unsafe-encoding
branch
from
d6783a9 to
34a771c
Compare
tmigone
force-pushed
the
horizon-oz2/l11-rewards-closed-allos
branch
from
93a3838 to
9fdcf37
Compare
pcarranzav
reviewed
May 1, 2025
| @@ -836,6 +843,9 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M | |||
| ); | |||
| } | |||
|
|
|||
| // Close the allocation | |||
| __allocations[_allocationID].closedAtEpoch = alloc.closedAtEpoch; | |||
There was a problem hiding this comment.
The scary thing here is it breaks the checks-effects-interactions pattern... I don't see any reentrancy vectors, as I don't think we do any calls to untrusted addresses in this flow, but it's worth double checking, or reconsidering if it might not be better to make getRewards return 0 but takeRewards work as before
There was a problem hiding this comment.
Agreed, I noted this in the PR comment but perhaps worth doing the same in the code. I don't think there is reentrancy risk, but we can have OZ comment on which alternative they prefer, wdyt?
tmigone
force-pushed
the
horizon-oz2/l10-unsafe-encoding
branch
from
34a771c to
8808257
Compare
tmigone
force-pushed
the
horizon-oz2/l11-rewards-closed-allos
branch
from
9fdcf37 to
a76885d
Compare
Maikol
approved these changes
May 2, 2025
tmigone
force-pushed
the
horizon-oz2/l10-unsafe-encoding
branch
2 times, most recently
from
6811eb0 to
dbc4f37
Compare
tmigone
force-pushed
the
horizon-oz2/l11-rewards-closed-allos
branch
2 times, most recently
from
568a714 to
7db0ae7
Compare
tmigone
force-pushed
the
horizon-oz2/l10-unsafe-encoding
branch
from
dbc4f37 to
c2a753d
Compare
…L-11) Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
tmigone
force-pushed
the
horizon-oz2/l11-rewards-closed-allos
branch
from
7db0ae7 to
e053527
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tricky bit in
HorizonStakingExtension.solis moving theclosedAtEpochstorage update after theRewardsManagerinternal calls, here: https://github.com/graphprotocol/contracts/pull/1149/files#diff-f0ca33d417ab44fd34813873b594fcfb2dcb852a3bf77c8c3b8ccb88130d9797R378-R380