search

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

1 stars 1 forks source link

smartWallet address is not guaranteed correct. ETH may be lost #317

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/SavETHVault.sol#L206-L207 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/SavETHVault.sol#L209

Vulnerability details

Impact

Liquid staking manager call function withdrawETHForStaking(address _smartWallet, uint256 _amount) to withdraw ETH for staking. It's manager's responsibility to set the correct _smartWallet address. However, there is no way to guarantee this. If a typo (or any other reasons) leads to a non-zero non-existent _smartWallet address, this function won't be able to detect the problem, and the ETH transfer statement will always return true. This will result in the ETH permanently locked to a non-existent account.

Proof of Concept

Liquid staking manager call function withdrawETHForStaking(address _smartWallet, uint256 _amount) with a non-zero non-existent _smartWallet address and some _amount of ETH. Function call will succeed but the ETH will be locked to the non-existent _smartWallet address.

Tools Used

Manual audit.

Recommended Mitigation Steps

The problem can be solved if we can verify the _smartWallet is a valid existent smartWallet before ETH transfer. The easiest solution is to verify the smartWallet has a valid owner since the smart wallet we are using is ownable. So, just add the checking owner code before ETH transfer.

c4-judge commented 1 year ago

dmvt marked the issue as primary issue

dmvt commented 1 year ago

As with #308, I recommend that the sponsor review all of the duplicates of this issue.

c4-sponsor commented 1 year ago

vince0656 marked the issue as sponsor confirmed

c4-judge commented 1 year ago

dmvt marked the issue as selected for report

c4-judge commented 1 year ago

dmvt marked the issue as satisfactory

trust1995 commented 1 year ago

I believe lack of existence check on passed address is very deep into QA/L territory, as has been standardized in the org - https://github.com/code-423n4/org/issues/53 Requires a massive user mistake, imo rewarding these kinds of findings as M dilutes the pool and does injustice to proper Ms. @GalloDaSballo

dmvt commented 1 year ago

See my response in the post-judging qa discussion.