fix: address inconsistent parameter and mapping order (OZ N-05)

[MoonBoi9001]

Motivation:

• This PR addresses the following N-05 OZ audit issue, applying the recommended fixes.

Title:

N-05 Inconsistent Parameter and Mapping Order

Details:

In the HorizonStaking contract, the _isAuthorized function has the following parameter order: _operator, _serviceProvider, _verifier. On the other hand, the _operatorAuth mapping uses this order: _serviceProvider, _verifier, _operator. In addition, the _legacyOperatorAuth variable is defined as a mapping from an operator to a service provider to a boolean status. However, it is being used as a mapping from a service provider to an operator to a boolean status.

These inconsistencies can lead to confusion and potential errors during integration or modification, as developers may misinterpret the correct parameter order.

Consider aligning the parameter order in both the _isAuthorized function and the _operatorAuth mapping for improved consistency. Moreover, consider updating the keys and value names in the _legacyOperatorAuth mapping definition to match its usage.

Key changes:

• Aligned the parameter order in the _isAuthorized function
• Aligned the parameter order in the isAuthorized function
• Aligned the parameter order in the _operatorAuth function
• Aligned the parameter order in the ProvisionManagerNotAuthorized function
• Aligned the parameter order in the HorizonStakingNotAuthorized function
• Aligned the parameter order in the SetOperator function
• Aligned the parameter order in the SetOperatorLocked function

Consistent peramater order:

• ServiceProvider, Verifier, Operator

[MoonBoi9001]

Did not address:

• In addition, the _legacyOperatorAuth variable is defined as a mapping from an operator to a service provider to a boolean status. However, it is being used as a mapping from a service provider to an operator to a boolean status.

  • This point was already addressed.

• Moreover, consider updating the keys and value names in the _legacyOperatorAuth mapping definition to match its usage.

  • I'm unsure what changes need to happen given:
  • mapping(address serviceProvider => mapping(address legacyOperator => bool authorized)) internal _legacyOperatorAuth;
  • I presume the suggestion is to change authorized to allowed? See here

[openzeppelin-code]

fix: address inconsistent parameter and mapping order (OZ N-05)

Generated at commit: 0f1522d58427e292aa8d596b8657e2c303a63f7d

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	2 4 0 15 40 61
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

---

For more details view the full report in OpenZeppelin Code Inspector

[tmigone]

We should also update isAuthorized() external function, and probably in the tests/ folder there are cases where we call this function and we need to switch the param order as well. For example:

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

Line 758 in 95b3f83

bool beforeOperatorAllowedGetter = staking.isAuthorized(operator, msgSender, verifier);

[MoonBoi9001]

Presumably this would also imply updating the relative ordering of many other things too, for example:

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

Line 757 in 95b3f83

bool beforeOperatorAllowed = _getStorage_OperatorAuth(msgSender, operator, verifier, legacy);

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

Line 881 in 95b3f83

function setOperator(address operator, address verifier, bool allowed) external;

contracts/packages/horizon/contracts/data-service/utilities/ProvisionManager.sol

Line 87 in 95b3f83

_graphStaking().isAuthorized(msg.sender, serviceProvider, address(this)),

contracts/packages/horizon/contracts/data-service/utilities/ProvisionManager.sol

Line 88 in 95b3f83

ProvisionManagerNotAuthorized(msg.sender, serviceProvider)

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

Line 59 in 95b3f83

HorizonStakingNotAuthorized(msg.sender, serviceProvider, verifier)

Proposed consistent order: (ServiceProvider, Verifier, Operator)
New proposed consistent order: (ServiceProvider, Operator, Verifier)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.51%. Comparing base (e33bf12) to head (edc5c3e).
Report is 37 commits behind head on horizon.

Additional details and impacted files

@@           Coverage Diff            @@
##           horizon    #1034   +/-   ##
========================================
  Coverage    92.51%   92.51%           
========================================
  Files           46       46           
  Lines         2392     2392           
  Branches       428      428           
========================================
  Hits          2213     2213           
  Misses         179      179           

Flag	Coverage Δ
unittests	92.51% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tmigone]

Yes good idea being consistent. I wouldn't mind keeping _getStorage_OperatorAuth as is because its an internal test only function, but anything that's externally accessible it does seem like a good idea to be consistent.

[MoonBoi9001]

New Proposed consistent order after discussions with Pablo:

(ServiceProvider, Operator, Verifier)

[tmigone]

Hey @MoonBoi9001 @pcarranzav curious why you chose (ServiceProvider, Operator, Verifier) over (ServiceProvider, Verifier, Operator)? All the other staking functions follow the (ServiceProvider, Verifier, ...) pattern

[pcarranzav]

Hey @MoonBoi9001 @pcarranzav curious why you chose (ServiceProvider, Operator, Verifier) over (ServiceProvider, Verifier, Operator)? All the other staking functions follow the (ServiceProvider, Verifier, ...) pattern

I don't feel strongly about it, but to me it made intuitive sense that:

• Operator and service provider are closely related, so it made sense to have them close to each other
• It is coherent with other functions having (serviceProvider, verifier) because the ordering is transitive, i.e. (serviceProvider, operator), (operator, verifier), (serviceProvider, verifier) are valid orderings for functions that have only two of the three. And we already have some functions with (operator, verifier).

But if you think the other ordering is cleaner I'm happy with that too; but would prefer if the "transitiveness" was enforced, e.g. setOperator and setOperatorLocked should be changed to (verifier, operator, allowed) as well

[tmigone]

I see. I think I still prefer to keep the (serviceProvider, verifier, ...) pattern, I feel like it's easier to remember, but I do agree we need to change SetOperator and SetOperatorLocked as well to enforce the transitiveness.

[pcarranzav]

I'm happy with that, @MoonBoi9001 sorry for the churn, how do you feel about changing it to (serviceProvider, verifier, operator) and checking it's transitively consistent everywhere?

[MoonBoi9001]

Okay, will do that.

[pcarranzav]

Looks great 👍

[tmigone]

thanks @MoonBoi9001 and sorry for the trouble 👍