search

code-423n4 / 2022-11-stakehouse-findings

1 stars 1 forks source link

`rotateNodeRunnerOfSmartWallet()` should be more solidified #354

Closed code423n4 closed 1 year ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L356-L377

Vulnerability details

Impact

rotateNodeRunnerOfSmartWallet() is used to change the current node runner. However, if the _new address is a non-zero non-existent account due to typo or other reasons, the nodeRunner will be tranfserred to an inaccessible non-existent account. Although it can be rotated to a right one by the dao, it will create unfriendly customer experience.

Proof of Concept

rotateNodeRunnerOfSmartWallet() is called with a non-zero non-existent _new account, the call will succeed as _new will pass all tests. However, the nodeRunner is transferred to a non-accessible account.

Tools Used

Manual audit.

Recommended Mitigation Steps

Take a two-step role change pattern. Step 1: nodeRunner or dao register the potential nodeRunner in the contract; Step2: implement a function, which can be called only by the potential nodeRunner, for the potential nodeRunner to replace the current nodeRunner with itself.

dmvt commented 1 year ago

Lack of two step process for address changes is considered QA.

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Overinflated severity