Closed code423n4 closed 2 years ago
By removing a vault, you remove the ability to interact with it, however funds are not freezed. Funds are stored in the NestedReserve
, you can withdraw
(function from NestedFactory
) from there and get back the share token in your wallet to withdraw by yourself using the Beefy UI or Yearn UI.
Lines of code
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/BeefyVaultStorage.sol#L32-L38 https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnVaultStorage.sol#L39-L45
Vulnerability details
Vault removal methods don't check if there are any funds still invested with the vault being removed. In the same time after vault was removed withdraws from it will not be available as non-zero checks in the corresponding functions will fail.
Setting severity to medium as this it principal fund freeze impact conditional on operational mistake of removing still active vault which is now possible.
Proof of Concept
BeefyVault can be removed while there still are leftover funds invested with it:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/BeefyVaultStorage.sol#L32-L38
Same for Yearn vault, it can be removed while still utilized by the users:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnVaultStorage.sol#L39-L45
In both cases any user funds remaining are frozen with the vaults removed as non-zero vault configuration checks will revert any withdrawal attempts:
https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnCurveVaultOperator.sol#L254-L263
Recommended Mitigation Steps
Consider introducing the check for the current vault token balance to the both removeVault functions.
Also, an argument of force removal can be added. For example, if forceRemoval is true, the logic is as it currently is, just remove the vault, if forceRemoval is false check for vault token balance and remove the vault if the amount is lower than a predefined dust threshold, reverting otherwise.