0x73696d616f - Frozen/paused Market that is harvested from in StakedEXA will DoS deposits leading to loss of yield

[sherlock-admin2]

0x73696d616f

Medium

Frozen/paused Market that is harvested from in StakedEXA will DoS deposits leading to loss of yield

Summary

StakedEXA::harvest() withdraws from the market of the StakedEXA contract if the provider has assets there and has approved StakedEXA. However, the market may be paused/frozen, but StakedEXA::harvest() will try to withdraw anyway, which will make it revert. As it is part of the deposit flow, it means all deposits will revert and new users will not be able to collect yield.

Root Cause

In StakedEXA::_update(), StakedEXA::harvest() is called if it is a mint (from == address(0)), here. Then. in StakedEXA:harvest(), it withdraws from the market regardless of its state, reverting if frozen or paused.

Internal pre-conditions

None.

External pre-conditions

1. Market is frozen or paused.

Attack Path

1. Market is frozen or paused.
2. StakedEXA deposits are frozen.

Impact

Frozen StakedEXA deposits which means new users will miss out on the yield and current users will enjoy a guaranteed higher yield.

PoC

Add the following test to StakedEXA.t.sol confirming that deposits are halted when the market is paused.

function test_POC_halted_deposits() external {
  uint256 assets = 1e18;

  market = Market(address(new ERC1967Proxy(address(new Market(ERC20Solmate(address(providerAsset)), new Auditor(18))), "")));
  market.initialize(
    "STEXA",
    3,
    1e18,
    new InterestRateModel(
      IRMParameters({
        minRate: 3.5e16,
        naturalRate: 8e16,
        maxUtilization: 1.1e18,
        naturalUtilization: 0.75e18,
        growthSpeed: 1.1e18,
        sigmoidSpeed: 2.5e18,
        spreadFactor: 0.2e18,
        maturitySpeed: 0.5e18,
        timePreference: 0.01e18,
        fixedAllocation: 0.6e18,
        maxRate: 15_000e16
    }),
    Market(address(0))),
    0.02e18 / uint256(1 days),
    1e17,
    0,
    0.0046e18,
    0.42e18
  );
  market.grantRole(market.PAUSER_ROLE(), address(this));

  vm.prank(address(stEXA));
  providerAsset.approve(address(market), type(uint256).max);

  stEXA.setMarket(market);

  providerAsset.mint(PROVIDER, 1_000e18);
  vm.startPrank(PROVIDER);
  providerAsset.approve(address(market), type(uint256).max);
  market.deposit(1_000e18, PROVIDER);
  market.approve(address(stEXA), 1_000e18);
  vm.stopPrank();

  market.pause();

  exa.mint(address(this), assets);

  vm.expectRevert();
  stEXA.deposit(assets, address(this));
}

Mitigation

Check if the market is paused or frozen and do not harvest if this is the case.

[santipu03]

Escalate

I believe this issue should be low severity according to the following rule from the Sherlock docs:

An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

[sherlock-admin3]

Escalate

I believe this issue should be low severity according to the following rule from the Sherlock docs:

An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

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

[0xsimao]

The rule doesn't apply here because the concerns are in different contracts. It's not StakedEXA that is paused, but the Market integration itself. StakedEXA should keep working normally.

[WangSecurity]

As I understand market is external and it's the external admin pausing the market, not Exactly admin, correct? If the market is frozen/paused, why the user should be able to deposit into that market?

[0xsimao]

@WangSecurity the market is also a contract of the Exactly protocol. The relevant part is that each contract, Market and StakedEXA, have their own admin and pausing functionalities. Harvesting is one functionality of StakedEXA and it should not DoS deposits, as StakedEXA fully works regardless of the market's state. StakedEXA has its own pausing functionality here.

Answering your question, the purpose of StakedEXA is staking the EXA, the selected market is just chosen to get the rewards from the provider to the users staking, the EXA staked is not actually deposited into the market. Thus it's crucial users can continue staking even if the current selected market for harvesting gets paused/frozen. Rewards are still being accumulated from previous harvests (the duration is 24 weeks). Thus, it would make no sense to forcibly DoS staking as users should be able to deposit and benefit from this yield and the admin of StakedEXA should set a new market or wait for the market to be live again, not stop staking.

[WangSecurity]

Can you share where you got this information from? I mean the fact that the users must be able to deposit at any time even if the market is paused/frozen, is it from README, code comments or docs? I don't see it in the first two, but I might be missing something?

[0xsimao]

@WangSecurity EXA that is deposited in StakedEXA is not deposited in the market, it stays in the contract and accumulates rewards from the provider. The provider is the one that deposits in the market but this is external to StakedEXA. The contract is meant to lock liquidity and benefit users that hold their EXA.

It would make sense to freeze deposits if a market is frozen if the EXA tokens were deposited to the market when deposited in StakedEXA, but it is not the case.

Feel free to go through the StakedEXA code, nowhere does it deposit user deposits in the market. It only deposits the provider funds in the market to the savings when the provider gives allowance, but user EXA deposits stay in StakedEXA. Thus, as the EXA funds are not deposited in the market, deposits should not be DoSed when the market is frozen.

[WangSecurity]

@0xsimao as I understand, you mean that users should be able to deposit into StakedEXA at any moment in time, correct? Can you share what this understanding is based on? Is it documented or based on how the code functions (excuse me if a silly question, just don't see it in the code comments or README)?

[0xsimao]

@WangSecurity yes, that is correct.

The harvesting functionality should not DoS staking ever, there may be periods without new rewards from harvest(), the provider chooses when/how much rewards to distribute. Take a look at this comment here, which says:
This function withdraws the maximum allowable assets from the provider's market,
If the market is paused, the maximum allowable assets should be 0. However, it tries to withdraw anyway and reverts.

[WangSecurity]

Thank you for these clarifications. I see how the concern about admin actions has got here. But, indeed, the users should be able to deposit in StakedEXA under any circumstances and when depositing into StakedEXA, users' funds indeed stay inside the contract and are not deposited inside the market. Additionally, both Market and StakedEXA have different admins and pausing functionalities. That's why I agree the rule shouldn't apply here and the issue should remain a valid medium. Planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result:
Medium
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• santipu03: rejected

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
exactly/protocol#751

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.