code-423n4 / 2022-01-sandclock-findings

0 stars 0 forks source link

[WP-H9] Centralization Risk: Funds can be frozen when critical key holders lose access to their keys #165

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

The current implementation requires trusted key holders (isTrusted[msg.sender]) to send transactions (initRedeemStable()) to initialize withdrawals from EthAnchor before the users can withdraw funds from the contract.

https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/strategy/BaseStrategy.sol#L214-L223

https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/strategy/BaseStrategy.sol#L163-L170

This introduces a high centralization risk, which can cause funds to be frozen in the contract if the key holders lose access to their keys.

PoC

Given:

If the key holders lose access to their keys ("hit by a bus"). The 800k will be frozen in EthAnchor as no one can initRedeemStable().

Recommendation

See the recommendation on issue [WP-M1].

0xBRM commented 2 years ago

Agree that there should be a way for users to call the uninvest functions themselves, subject to certain rules. Again, not sure I agree with the severity given the likelihood of the event transpiring.

Consensus is for UST vaults, allow depositors to call uninvest. For nonUST vaults that pay per curve swap, add trusted multisig instead of just the backend's EOA.

dmvt commented 2 years ago

This issue requires external factors to align in a very negative way, but it would result in a potentially significant loss of funds. Because there is no direct attack path, it doesn't qualify as a high risk issue, but a medium risk per Code4rena definitions.

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.