When a user calls withdraw, the amount of underlying assets that they are owed is dependent on their number of shares, and the number of underlying assets in the vault + strategy. If x is the number of underlying tokens intended to be sent to the user in a call to withdraw, then it is certainly possible that the balance of the vault is less than x (as most of the tokens are locked up in the strategy).
In this case, whenever the user calls withdraw, the call will revert, and there will need to be manual intervention from an admin to transfer the appropriate amount of tokens from the strategy back to the vault using withdrawToVault.
This is not very ideal, and this is actually an issue, since users will not be able to withdraw from their positions when they want - they will need to rely on manual intervention from the admins. Once the lockedUntil timestamp passes, a user should be able to withdraw from their position whenever they desire.
Notice that there are no calls to the strategy here, so if there are not the correct number of underlying tokens in the strategy for the user, the call to withdraw will revert.
Tools Used
Inspection.
Recommended Mitigation Steps
The vault should determine if extra tokens are needed to be withdrawn from the strategy, and if necessary the vault should call withdrawToVault to get tokens from the stategy. This way, users will always be able to withdraw from their positions once their lockedUntil timestamp has expired.
naps62
commented
2 years ago
Handle
harleythedog
Vulnerability details
Impact
When a user calls
withdraw, the amount of underlying assets that they are owed is dependent on their number of shares, and the number of underlying assets in the vault + strategy. Ifxis the number of underlying tokens intended to be sent to the user in a call towithdraw, then it is certainly possible that the balance of the vault is less thanx(as most of the tokens are locked up in the strategy).In this case, whenever the user calls
withdraw, the call will revert, and there will need to be manual intervention from an admin to transfer the appropriate amount of tokens from the strategy back to the vault usingwithdrawToVault.This is not very ideal, and this is actually an issue, since users will not be able to withdraw from their positions when they want - they will need to rely on manual intervention from the admins. Once the
lockedUntiltimestamp passes, a user should be able to withdraw from their position whenever they desire.Proof of Concept
See the code for
_withdrawhere: https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/Vault.sol#L325Notice that there are no calls to the strategy here, so if there are not the correct number of underlying tokens in the strategy for the user, the call to
withdrawwill revert.Tools Used
Inspection.
Recommended Mitigation Steps
The vault should determine if extra tokens are needed to be withdrawn from the strategy, and if necessary the vault should call
withdrawToVaultto get tokens from the stategy. This way, users will always be able to withdraw from their positions once theirlockedUntiltimestamp has expired.