The signatures check of`approveValidators()` and `updateExitSignatures()` can be bypassed as long as `validatorsMinOracles` is not set

[hats-bug-reporter[bot]]

Github username: -- Submission hash (on-chain): 0x721c5dcbecb51366d38e8422d56a41daca9ab4de225ca50277cc5b9d18319d3b Severity: medium

Description: In KeeperValidators.sol, there is a function approveValidators() that calls internal _verifySignatures() and provides validatorsMinOracles as the parameter. However, if it's not set yet, the validators will be approved without any oracle signature.

Same goes for updateExitSignatures() which calls _verifySignatures() by providing validatorsMinOracles as the parameter. If it's not set yet, the exitSignaturesNonces of the vault will be updated without any oracle signature.

Attack Scenario

• User provides params inside of approveValidators() function and front-runs setValidatorsMinOracles() (maybe the owner wasn't able to call it right away due to high network congestion or high gas costs). The user can approve validators basically without any oracle signatures because they will be set to 0.

• User provides params inside of updateExitSignatures() function and front-runs setValidatorsMinOracles() the same way. The user can update the exit signature of a vault basically without any oracle signatures because they will be set to 0.

In both cases, the check inside of _verifySignatures() is bypassed:

https://github.com/stakewise/v3-core/blob/c82fc57d013a19967576f683c5e41900cbdd0e67/contracts/keeper/KeeperOracles.sol#L86-L88 as requiredSignatures == 0

_verifySignatures() is internal view function that doesn't return anything (bool, for example), so after the call the state is updated as _verifySignatures() is not reverted.

Recommendation

validatorsMinOracles should be set in the constructor right away and then use setValidatorsMinOracles() function as the ability to update the value.

[tsudmi]

Duplicate of https://github.com/hats-finance/StakeWise-0xd91cd6ed6c9a112fdc112b1a3c66e47697f522cd/issues/13

[MiniGlome]

Ok I copied/pasted a bit of the wording of issue#13 to save time (because time is very precious in this type of contest) but this issue is not the same issue. This issue is in a different contract (KeeperValidators not KeeperRewards) and affects different functions (approveValidators and updateExitSignatures).

It's like saying there is a re-entrancy in contract A. If someone reports a re-entrancy in contract B then this is not the same issue.

[MiniGlome]

@tsudmi

[tsudmi]

Hey @MiniGlome the issue here is not with KeeperValidators or KeeperRewards, but with KeeperOracles that both modules inherit from, so whatever module that is inherited from KeeperOracles will have that issue. The fix can be seen here: https://github.com/stakewise/v3-core/blob/hats-fixes/contracts/keeper/KeeperOracles.sol#L83