Open c4-bot-5 opened 2 months ago
Ideally that's bestowed upon the DSS on how to handle it, the reason for ignoring the failure is to prevent a malicious DSS from blocking unregistering/unstaking the operator from DSS. @dewpe
Would reclassify as low
because the unregistration hook itself has no guarantee of running successfully hence the allow revert flag being enabled. Ultimately the implementation of the hooks is up to the DSS and the best we can provide is a recommendation to try-catch
inside the handler for this specific hook when they are taking in custom data from the unregistrationHookData
param just incase junk data is passed in
@devdks25 Sponsors can only use these labels: sponsor confirmed, sponsor disputed, sponsor acknowledged.
MiloTruck changed the severity to QA (Quality Assurance)
Agree with the sponsor.
This issue and its duplicates rely on speculation on future code - the DSS implementation is not in-scope. It is the responsibility of the DSS to handle the case where finalizeUpdateVaultStakeInDSS()
does not revert even when finishUpdateStakeHook()
does.
Therefore, I believe this is QA at best.
MiloTruck marked the issue as grade-b
Lines of code
https://github.com/code-423n4/2024-07-karak/blob/53eb78ebda718d752023db4faff4ab1567327db4/src/Core.sol#L113-L124 https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/Operator.sol#L181-L203 https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/HookLib.sol#L78-L103 https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/HookLib.sol#L16-L39
Vulnerability details
Impact
When calling the following
Core.unregisterOperatorFromDSS
function, the specifiedunregistrationHookData
is used as an input for calling the DSS'sunregistrationHook
function. When the DSS needsunregistrationHookData
to contain certain content or be encoded for certain types, the operator can specifyunregistrationHookData
in which the content or types resulted from decoding suchunregistrationHookData
in the DSS'sunregistrationHook
function do not match the DSS's expected content or types, causing the DSS'sunregistrationHook
function to revert. Alternatively, the operator can specifyunregistrationHookData
to be memory-intensive enough to cause the processing of the specifiedunregistrationHookData
to consume more gas thanself.hookCallGasLimit
, which also reverts the DSS'sunregistrationHook
function.Since the
Core.unregisterOperatorFromDSS
function calls theHookLib.callHookIfInterfaceImplemented
function with theignoreFailure
input beingtrue
, reverting the DSS'sunregistrationHook
function call does not revert the operator'sCore.unregisterOperatorFromDSS
function call. However, the DSS'sunregistrationHook
function's logics can be important to such DSS, such as for keeping correct accounting of the operators that have been unregistered from such DSS. As a result, the operator is able to DOS the DSS'sunregistrationHook
function by specifying arbitraryunregistrationHookData
while not reverting its ownCore.unregisterOperatorFromDSS
function call.https://github.com/code-423n4/2024-07-karak/blob/53eb78ebda718d752023db4faff4ab1567327db4/src/Core.sol#L113-L124
https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/Operator.sol#L181-L203
https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/HookLib.sol#L78-L103
https://github.com/code-423n4/2024-07-karak/blob/d19a4de35bcaf31ccec8bccd36e2d26594d05aad/src/entities/HookLib.sol#L16-L39
Proof of Concept
The following steps can occur for the described scenario.
unregistrationHook
function contains logics for keeping track of the operators that have been unregistered from DSS A and decoding the receivedunregistrationHookData
into separate variables for emitting events.abi.encodePacked
function to create theunregistrationHookData
and provides suchunregistrationHookData
when calling theCore.unregisterOperatorFromDSS
function.Core.unregisterOperatorFromDSS
transaction calls DSS A'sunregistrationHook
function, decoding suchunregistrationHookData
reverts, which also reverts DSS A'sunregistrationHook
function.Core.unregisterOperatorFromDSS
function calls theHookLib.callHookIfInterfaceImplemented
function with theignoreFailure
input beingtrue
, reverting DSS A'sunregistrationHook
function call does not revert Operator A'sCore.unregisterOperatorFromDSS
transaction.Core.unregisterOperatorFromDSS
transaction succeeds.Tools Used
Manual Review
Recommended Mitigation Steps
The
Core.unregisterOperatorFromDSS
function can be updated to disallow the operator from specifying arbitraryunregistrationHookData
; instead, theCore.unregisterOperatorFromDSS
function can specify a determined set of input variables for calling the DSS'sunregistrationHook
function so the DSS would implement itsunregistrationHook
function in a way that accommodates such input variables.Assessed type
DoS