hats-finance/Origami-0x998f1b716a5022be026ca6b919c0ddf45ca31abd

When we update the old IdleStrategy, there may be some USDC tokens in that.

Opened this issue · 1 comments

Github username: --
Twitter username: --
Submission hash (on-chain): 0xcfe892ae77881976899587709036d34932275abb12a0a6076b9c9cfdd5bb4558
Severity: low

Description:
Description
An elevated user has the ability to update the IdleStrategy in the OrigamiIdleStrategyManager.
If there is an old IdleStrategy, we enforce approval of 0 for the old one.
However, we do not check whether there are any allocated USDC tokens in it.

Attack Scenario
The IdleStrategyManager has previously interacted with the old IdleStrategy, O_I, and some USDC tokens were allocated to it.
Now, we are preparing to update to the new IdleStrategy, N_I.
The following function is invoked:

function setIdleStrategy(address _idleStrategy) external override onlyElevatedAccess {
    if (_idleStrategy == address(0)) revert CommonEventsAndErrors.InvalidAddress(_idleStrategy);

    address _oldStrategy = address(idleStrategy);
    if (_oldStrategy != address(0)) {
        asset.forceApprove(_oldStrategy, 0);
    }
    asset.forceApprove(_idleStrategy, type(uint256).max);

    idleStrategy = IOrigamiIdleStrategy(_idleStrategy);
    emit IdleStrategySet(_idleStrategy);
}

There is no verification conducted to confirm if there are any allocated USDC tokens in the O_I.

Additional actions are required to utilize those USDC tokens.

Attachments

  1. Proof of Concept (PoC) File
  1. Revised Code File (Optional)

Implement functionality to withdraw allocated USDC tokens from the old IdleStrategy.

function setIdleStrategy(address _idleStrategy) external override onlyElevatedAccess {
    if (_idleStrategy == address(0)) revert CommonEventsAndErrors.InvalidAddress(_idleStrategy);

    address _oldStrategy = address(idleStrategy);
    if (_oldStrategy != address(0)) {
        asset.forceApprove(_oldStrategy, 0);

+         uint256 amount = IOrigamiIdleStrategy(_oldStrategy).availableToWithdraw();

+         if (amount != 0) {
+             IOrigamiIdleStrategy(_oldStrategy).withdraw(amount, address(this));
+         }
    }
    asset.forceApprove(_idleStrategy, type(uint256).max);

    idleStrategy = IOrigamiIdleStrategy(_idleStrategy);
    emit IdleStrategySet(_idleStrategy);
}

Thanks for raising

There is no verification conducted to confirm if there are any allocated USDC tokens in the O_I.

This is intentional. If the old idle strategy is compromised we want an immediate way to cut over ASAP. If the check was there we would never be able to cut it over.