Skip to content

Commit 0a7719f

Browse files
tmigonetmigone
committed
fix: prevent provision validity grief attack (OZ M-03)
Signed-off-by: Tomás Migone <[email protected]>
1 parent 5e27d62 commit 0a7719f

File tree

4 files changed

+184
-15
lines changed

4 files changed

+184
-15
lines changed

‎packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -541,10 +541,10 @@ interface IHorizonStakingMain {
541541
*/
542542
function stakeTo(address serviceProvider, uint256 tokens) external;
543543

544-
// can be called by anyone if the service provider has provisioned stake to this verifier
545544
/**
546545
* @notice Deposit tokens on the service provider stake, on behalf of the service provider,
547546
* provisioned to a specific verifier.
547+
* @dev This function can be called by the service provider, by an authorized operator or by the verifier itself.
548548
* @dev Requirements:
549549
* - The `serviceProvider` must have previously provisioned stake to `verifier`.
550550
* - `_tokens` cannot be zero.

‎packages/horizon/contracts/staking/HorizonStaking.sol‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,19 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
5757
_;
5858
}
5959

60+
/**
61+
* @notice Checks that the caller is authorized to operate over a provision or it is the verifier.
62+
* @param serviceProvider The address of the service provider.
63+
* @param verifier The address of the verifier.
64+
*/
65+
modifier onlyAuthorizedOrVerifier(address serviceProvider, address verifier) {
66+
require(
67+
_isAuthorized(serviceProvider, verifier, msg.sender) || msg.sender == verifier,
68+
HorizonStakingNotAuthorized(serviceProvider, verifier, msg.sender)
69+
);
70+
_;
71+
}
72+
6073
/**
6174
* @dev The staking contract is upgradeable however we still use the constructor to set
6275
* a few immutable variables.
@@ -121,7 +134,11 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
121134
}
122135

123136
/// @inheritdoc IHorizonStakingMain
124-
function stakeToProvision(address serviceProvider, address verifier, uint256 tokens) external override notPaused {
137+
function stakeToProvision(
138+
address serviceProvider,
139+
address verifier,
140+
uint256 tokens
141+
) external override notPaused onlyAuthorizedOrVerifier(serviceProvider, verifier) {
125142
_stakeTo(serviceProvider, tokens);
126143
_addToProvision(serviceProvider, verifier, tokens);
127144
}

‎packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,62 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest {
181181
);
182182
}
183183

184+
function _stakeToProvision(address serviceProvider, address verifier, uint256 tokens) internal {
185+
(, address msgSender, ) = vm.readCallers();
186+
187+
// before
188+
uint256 beforeStakingBalance = token.balanceOf(address(staking));
189+
uint256 beforeSenderBalance = token.balanceOf(msgSender);
190+
ServiceProviderInternal memory beforeServiceProvider = _getStorage_ServiceProviderInternal(serviceProvider);
191+
Provision memory beforeProvision = staking.getProvision(serviceProvider, verifier);
192+
193+
// stakeTo
194+
token.approve(address(staking), tokens);
195+
vm.expectEmit();
196+
emit IHorizonStakingBase.HorizonStakeDeposited(serviceProvider, tokens);
197+
vm.expectEmit();
198+
emit IHorizonStakingMain.ProvisionIncreased(serviceProvider, verifier, tokens);
199+
staking.stakeToProvision(serviceProvider, verifier, tokens);
200+
201+
// after
202+
uint256 afterStakingBalance = token.balanceOf(address(staking));
203+
uint256 afterSenderBalance = token.balanceOf(msgSender);
204+
ServiceProviderInternal memory afterServiceProvider = _getStorage_ServiceProviderInternal(serviceProvider);
205+
Provision memory afterProvision = staking.getProvision(serviceProvider, verifier);
206+
207+
// assert - stakeTo
208+
assertEq(afterStakingBalance, beforeStakingBalance + tokens);
209+
assertEq(afterSenderBalance, beforeSenderBalance - tokens);
210+
assertEq(afterServiceProvider.tokensStaked, beforeServiceProvider.tokensStaked + tokens);
211+
assertEq(afterServiceProvider.tokensProvisioned, beforeServiceProvider.tokensProvisioned + tokens);
212+
assertEq(afterServiceProvider.__DEPRECATED_tokensAllocated, beforeServiceProvider.__DEPRECATED_tokensAllocated);
213+
assertEq(afterServiceProvider.__DEPRECATED_tokensLocked, beforeServiceProvider.__DEPRECATED_tokensLocked);
214+
assertEq(
215+
afterServiceProvider.__DEPRECATED_tokensLockedUntil,
216+
beforeServiceProvider.__DEPRECATED_tokensLockedUntil
217+
);
218+
219+
// assert - addToProvision
220+
assertEq(afterProvision.tokens, beforeProvision.tokens + tokens);
221+
assertEq(afterProvision.tokensThawing, beforeProvision.tokensThawing);
222+
assertEq(afterProvision.sharesThawing, beforeProvision.sharesThawing);
223+
assertEq(afterProvision.maxVerifierCut, beforeProvision.maxVerifierCut);
224+
assertEq(afterProvision.thawingPeriod, beforeProvision.thawingPeriod);
225+
assertEq(afterProvision.createdAt, beforeProvision.createdAt);
226+
assertEq(afterProvision.lastParametersStagedAt, beforeProvision.lastParametersStagedAt);
227+
assertEq(afterProvision.maxVerifierCutPending, beforeProvision.maxVerifierCutPending);
228+
assertEq(afterProvision.thawingPeriodPending, beforeProvision.thawingPeriodPending);
229+
assertEq(afterProvision.thawingNonce, beforeProvision.thawingNonce);
230+
assertEq(afterServiceProvider.tokensStaked, beforeServiceProvider.tokensStaked + tokens);
231+
assertEq(afterServiceProvider.tokensProvisioned, beforeServiceProvider.tokensProvisioned + tokens);
232+
assertEq(afterServiceProvider.__DEPRECATED_tokensAllocated, beforeServiceProvider.__DEPRECATED_tokensAllocated);
233+
assertEq(afterServiceProvider.__DEPRECATED_tokensLocked, beforeServiceProvider.__DEPRECATED_tokensLocked);
234+
assertEq(
235+
afterServiceProvider.__DEPRECATED_tokensLockedUntil,
236+
beforeServiceProvider.__DEPRECATED_tokensLockedUntil
237+
);
238+
}
239+
184240
function _unstake(uint256 _tokens) internal {
185241
(, address msgSender, ) = vm.readCallers();
186242

‎packages/horizon/test/staking/provision/provision.t.sol‎

Lines changed: 109 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -80,19 +80,6 @@ contract HorizonStakingProvisionTest is HorizonStakingTest {
8080
staking.provision(users.indexer, subgraphDataServiceAddress, amount / 2, maxVerifierCut, thawingPeriod);
8181
}
8282

83-
function testProvision_OperatorAddTokensToProvision(
84-
uint256 amount,
85-
uint32 maxVerifierCut,
86-
uint64 thawingPeriod,
87-
uint256 tokensToAdd
88-
) public useIndexer useProvision(amount, maxVerifierCut, thawingPeriod) useOperator {
89-
tokensToAdd = bound(tokensToAdd, 1, MAX_STAKING_TOKENS);
90-
91-
// Add more tokens to the provision
92-
_stakeTo(users.indexer, tokensToAdd);
93-
_addToProvision(users.indexer, subgraphDataServiceAddress, tokensToAdd);
94-
}
95-
9683
function testProvision_RevertWhen_OperatorNotAuthorized(
9784
uint256 amount,
9885
uint32 maxVerifierCut,
@@ -124,4 +111,113 @@ contract HorizonStakingProvisionTest is HorizonStakingTest {
124111
vm.expectRevert(expectedError);
125112
staking.provision(users.indexer, subgraphDataServiceAddress, amount, 0, 0);
126113
}
114+
115+
function testProvision_AddTokensToProvision(
116+
uint256 amount,
117+
uint32 maxVerifierCut,
118+
uint64 thawingPeriod,
119+
uint256 tokensToAdd
120+
) public useIndexer useProvision(amount, maxVerifierCut, thawingPeriod) {
121+
tokensToAdd = bound(tokensToAdd, 1, MAX_STAKING_TOKENS);
122+
123+
// Add more tokens to the provision
124+
_stakeTo(users.indexer, tokensToAdd);
125+
_addToProvision(users.indexer, subgraphDataServiceAddress, tokensToAdd);
126+
}
127+
128+
function testProvision_OperatorAddTokensToProvision(
129+
uint256 amount,
130+
uint32 maxVerifierCut,
131+
uint64 thawingPeriod,
132+
uint256 tokensToAdd
133+
) public useIndexer useProvision(amount, maxVerifierCut, thawingPeriod) useOperator {
134+
tokensToAdd = bound(tokensToAdd, 1, MAX_STAKING_TOKENS);
135+
136+
// Add more tokens to the provision
137+
_stakeTo(users.indexer, tokensToAdd);
138+
_addToProvision(users.indexer, subgraphDataServiceAddress, tokensToAdd);
139+
}
140+
141+
function testProvision_AddTokensToProvision_RevertWhen_NotAuthorized(
142+
uint256 amount,
143+
uint32 maxVerifierCut,
144+
uint64 thawingPeriod,
145+
uint256 tokensToAdd
146+
) public useIndexer useProvision(amount, maxVerifierCut, thawingPeriod) {
147+
tokensToAdd = bound(tokensToAdd, 1, MAX_STAKING_TOKENS);
148+
149+
// Add more tokens to the provision
150+
_stakeTo(users.indexer, tokensToAdd);
151+
152+
// use delegator as a non authorized operator
153+
vm.startPrank(users.delegator);
154+
bytes memory expectedError = abi.encodeWithSignature(
155+
"HorizonStakingNotAuthorized(address,address,address)",
156+
users.indexer,
157+
subgraphDataServiceAddress,
158+
users.delegator
159+
);
160+
vm.expectRevert(expectedError);
161+
staking.addToProvision(users.indexer, subgraphDataServiceAddress, amount);
162+
}
163+
164+
function testProvision_StakeToProvision(
165+
uint256 amount,
166+
uint32 maxVerifierCut,
167+
uint64 thawingPeriod,
168+
uint256 tokensToAdd
169+
) public useIndexer useProvision(amount, maxVerifierCut, thawingPeriod) {
170+
tokensToAdd = bound(tokensToAdd, 1, MAX_STAKING_TOKENS);
171+
172+
// Add more tokens to the provision
173+
_stakeToProvision(users.indexer, subgraphDataServiceAddress, tokensToAdd);
174+
}
175+
176+
function testProvision_Operator_StakeToProvision(
177+
uint256 amount,
178+
uint32 maxVerifierCut,
179+
uint64 thawingPeriod,
180+
uint256 tokensToAdd
181+
) public useIndexer useProvision(amount, maxVerifierCut, thawingPeriod) useOperator {
182+
tokensToAdd = bound(tokensToAdd, 1, MAX_STAKING_TOKENS);
183+
184+
// Add more tokens to the provision
185+
_stakeToProvision(users.indexer, subgraphDataServiceAddress, tokensToAdd);
186+
}
187+
188+
function testProvision_Verifier_StakeToProvision(
189+
uint256 amount,
190+
uint32 maxVerifierCut,
191+
uint64 thawingPeriod,
192+
uint256 tokensToAdd
193+
) public useIndexer useProvision(amount, maxVerifierCut, thawingPeriod) {
194+
tokensToAdd = bound(tokensToAdd, 1, MAX_STAKING_TOKENS);
195+
196+
// Ensure the verifier has enough tokens to then stake to the provision
197+
token.transfer(subgraphDataServiceAddress, tokensToAdd);
198+
199+
// Add more tokens to the provision
200+
resetPrank(subgraphDataServiceAddress);
201+
_stakeToProvision(users.indexer, subgraphDataServiceAddress, tokensToAdd);
202+
}
203+
204+
function testProvision_StakeToProvision_RevertWhen_NotAuthorized(
205+
uint256 amount,
206+
uint32 maxVerifierCut,
207+
uint64 thawingPeriod,
208+
uint256 tokensToAdd
209+
) public useIndexer useProvision(amount, maxVerifierCut, thawingPeriod) {
210+
tokensToAdd = bound(tokensToAdd, 1, MAX_STAKING_TOKENS);
211+
212+
// Add more tokens to the provision
213+
vm.startPrank(users.delegator);
214+
bytes memory expectedError = abi.encodeWithSignature(
215+
"HorizonStakingNotAuthorized(address,address,address)",
216+
users.indexer,
217+
subgraphDataServiceAddress,
218+
users.delegator
219+
);
220+
vm.expectRevert(expectedError);
221+
staking.stakeToProvision(users.indexer, subgraphDataServiceAddress, tokensToAdd);
222+
}
127223
}

0 commit comments

Comments
 (0)