ABA - `withdraw_underlying_to_claim` is callable by anybody; can be abused

[github-actions[bot]]

ABA

high

withdraw_underlying_to_claim is callable by anybody; can be abused

Summary

the function withdraw_underlying_to_claim is missing a check that the caller should be an operator (or other privileged role).

Vulnerability Detail

withdraw_underlying_to_claim withdraws _amount_shares and _min_weth_out from Alchemix to be distributed to token holders on claim.

Because anybody can execute withdraw_underlying_to_claim, a malicious actor can constantly call it, effectively invalidating any advantage that the Vault may have with using Alchemix.

Impact

The whole advantage of using Alchemix for self paying loans is invalidated, effectively removes it from the contract.

The collateral itself can only be extracted by each token holder, but the Alchemix position is destroyed.

Code Snippet

Function

@external
def withdraw_underlying_to_claim(_amount_shares: uint256, _min_weth_out: uint256):
    """
    @notice
        Withdraws _amount_shares and _min_weth_out from Alchemix to be distributed
        to token holders.
        The WETH is held in this contract until it is `claim`ed.
    """
    amount_withdrawn: uint256 = self._withdraw_underlying_from_alchemix(_amount_shares, self, _min_weth_out)
    self._mark_as_claimable(amount_withdrawn)

    log Claimable(amount_withdrawn)

Tool used

Manual Review

Recommendation

Add operator requirement for withdraw_underlying_to_claim.

@@ -398,6 +398,7 @@ def withdraw_underlying_to_claim(_amount_shares: uint256, _min_weth_out: uint256

+    assert self.is_operator[msg.sender], "unauthorized"
     amount_withdrawn: uint256 = self._withdraw_underlying_from_alchemix(_amount_shares, self, _min_weth_out)
     self._mark_as_claimable(amount_withdrawn)

ERC721 token holders can still withdrawal their own positions via the claim function, respecting the protocol public facing contract/scope.

[abarbatei]

Escalate for 10 USDC I think this is wrongly classified as a duplicate of #121 because they reffer to different vulnerabilities that appear from the same isssue (lack of access control)

In this case, I am reffering to the fact that any yield generating possitions on Alchemix will be destroyed by anyone at any time. Basically making this Vault a non-yield generating one, like it does not use Alchemix at all. You can basically throw away Alchemix at this point and it will be the same.

[sherlock-admin]

Escalate for 10 USDC I think this is wrongly classified as a duplicate of #121 because they reffer to different vulnerabilities that appear from the same isssue (lack of access control)

In this case, I am reffering to the fact that any yield generating possitions on Alchemix will be destroyed by anyone at any time. Basically making this Vault a non-yield generating one, like it does not use Alchemix at all. You can basically throw away Alchemix at this point and it will be the same.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Unstoppable-DeFi]

In this case, I am reffering to the fact that any yield generating possitions on Alchemix will be destroyed by anyone at any time. Basically making this Vault a non-yield generating one, like it does not use Alchemix at all. You can basically throw away Alchemix at this point and it will be the same.

This is not correct. Yield generating positions cannot be “destroyed” at any time.

When a new deposit is registered with the vault it 1) deposits into Alchemix 2) takes out a 50% loan in alETH and sends it to the fund receiver

At this point the LTV is at maximum, no shares can be withdrawn, nothing can be “destroyed”.

withdraw_underlying_to_claim can only be called after the yield has paid back (parts of) the loan. Paying back the loan (manually or automatically through yield generated by Alchemix) is what unlocks some of the shares to be withdrawable - at which point anybody can permissionlessly withdraw the unlocked collateral and make it claimable by initial depositors. This is the entire purpose of the protocol.

[hrishibhat]

Escalation accepted

While it is not a duplicate of #121, the underlying issue is still not valid based on the Sponsor comment above.

[sherlock-admin]

Escalation accepted

While it is not a duplicate of #121, the underlying issue is still not valid based on the Sponsor comment above.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.