Some Strategy functions can't be called from the Vault

[code423n4]

Handle

palina

Vulnerability details

Impact

BaseStrategy.sol contract implement modifiers (onlyVault() and restricted()) suggesting that its Vault should have the capability to invoke some of the restricted functionality, including withdrawAllToVault() and withdrawToVault().

However, according to the implementation, Vault.sol, contains only the call to Strategy's doHardWork() and investedAssets() (but not withdrawAllToVault() and withdrawToVault()). This suggests that this functionality might be missing in the implementation of Vault, which might lead to funds being locked in the strategy.

In BaseStrategy.sol, the defined onlyVault() modifier is never used.

Proof of Concept

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

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

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

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add the missing functionality (i.e., withdrawal from Strategy) to Vault or refine the modifiers in Strategy.sol to reflect the chosen role hierarchy (i.e., define the onlyGovernance() modifier to be applied to functions that are not intended to be called by a Vault; remove the onlyVault() modifier that is never used).

[naps62]

This was indeed an issue, but a low-risk one. The implementation works as intended in this regard, but became confusing after some refactors.

[dmvt]

Agreed on severity.