Skip to content

fix: only check conditions for param that is changing when changing provision params (OZ L-05) #1135

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

Open
wants to merge 1 commit into
base: horizon-oz2/n01-remove-code
Choose a base branch
from
Open

fix: only check conditions for param that is changing when changing provision params (OZ L-05) #1135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,10 +700,15 @@ interface IHorizonStakingMain {

/**
* @notice Stages a provision parameter update. Note that the change is not effective until the verifier calls
* {acceptProvisionParameters}.
* {acceptProvisionParameters}. Calling this function is a no-op if the new parameters are the same as the current
* ones.
* @dev This two step update process prevents the service provider from changing the parameters
* without the verifier's consent.
*
* Requirements:
* - `thawingPeriod` must be less than or equal to `_maxThawingPeriod`. Note that if `_maxThawingPeriod` changes the
* function will not revert if called with the same thawing period as the current one.
*
* Emits a {ProvisionParametersStaged} event if at least one of the parameters changed.
*
* @param serviceProvider The service provider address
Expand Down
25 changes: 16 additions & 9 deletions packages/horizon/contracts/staking/HorizonStaking.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,19 +219,26 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
uint32 newMaxVerifierCut,
uint64 newThawingPeriod
) external override notPaused onlyAuthorized(serviceProvider, verifier) {
require(PPMMath.isValidPPM(newMaxVerifierCut), HorizonStakingInvalidMaxVerifierCut(newMaxVerifierCut));
require(
newThawingPeriod <= _maxThawingPeriod,
HorizonStakingInvalidThawingPeriod(newThawingPeriod, _maxThawingPeriod)
);

// Provision must exist
Provision storage prov = _provisions[serviceProvider][verifier];
require(prov.createdAt != 0, HorizonStakingInvalidProvision(serviceProvider, verifier));

if ((prov.maxVerifierCutPending != newMaxVerifierCut) || (prov.thawingPeriodPending != newThawingPeriod)) {
prov.maxVerifierCutPending = newMaxVerifierCut;
prov.thawingPeriodPending = newThawingPeriod;
bool verifierCutChanged = prov.maxVerifierCutPending != newMaxVerifierCut;
bool thawingPeriodChanged = prov.thawingPeriodPending != newThawingPeriod;

if (verifierCutChanged || thawingPeriodChanged) {
if (verifierCutChanged) {
require(PPMMath.isValidPPM(newMaxVerifierCut), HorizonStakingInvalidMaxVerifierCut(newMaxVerifierCut));
prov.maxVerifierCutPending = newMaxVerifierCut;
}
if (thawingPeriodChanged) {
require(
newThawingPeriod <= _maxThawingPeriod,
HorizonStakingInvalidThawingPeriod(newThawingPeriod, _maxThawingPeriod)
);
prov.thawingPeriodPending = newThawingPeriod;
}

prov.lastParametersStagedAt = block.timestamp;
emit ProvisionParametersStaged(serviceProvider, verifier, newMaxVerifierCut, newThawingPeriod);
}
Expand Down
35 changes: 35 additions & 0 deletions packages/horizon/test/staking/provision/parameters.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,41 @@ contract HorizonStakingProvisionParametersTest is HorizonStakingTest {
staking.setProvisionParameters(users.indexer, subgraphDataServiceAddress, maxVerifierCut, thawingPeriod);
}

function test_ProvisionParametersSet_MaxMaxThawingPeriodChanged(
uint256 amount,
uint32 maxVerifierCut,
uint64 thawingPeriod
) public useIndexer {
vm.assume(amount > 0);
vm.assume(amount <= MAX_STAKING_TOKENS);
vm.assume(maxVerifierCut <= MAX_PPM);

// create provision with initial parameters
uint32 initialMaxVerifierCut = 1000;
uint64 initialThawingPeriod = 14 days; // Max thawing period is 28 days
_createProvision(users.indexer, subgraphDataServiceAddress, amount, initialMaxVerifierCut, initialThawingPeriod);

// change the max thawing period allowed so that the initial thawing period is not valid anymore
uint64 newMaxThawingPeriod = 7 days;
resetPrank(users.governor);
_setMaxThawingPeriod(newMaxThawingPeriod);

// set the verifier cut to a new value - keep the thawing period the same, it should be allowed
resetPrank(users.indexer);
_setProvisionParameters(users.indexer, subgraphDataServiceAddress, maxVerifierCut, initialThawingPeriod);

// now try to change the thawing period to a new value that is invalid
vm.assume(thawingPeriod > initialThawingPeriod);
vm.expectRevert(
abi.encodeWithSelector(
IHorizonStakingMain.HorizonStakingInvalidThawingPeriod.selector,
thawingPeriod,
newMaxThawingPeriod
)
);
staking.setProvisionParameters(users.indexer, subgraphDataServiceAddress, maxVerifierCut, thawingPeriod);
}

function test_ProvisionParametersAccept(
uint256 amount,
uint32 maxVerifierCut,
Expand Down