santipu_ - DoS on deposits when a low savings amount is harvested

[sherlock-admin3]

santipu_

Medium

DoS on deposits when a low savings amount is harvested

Summary

The missing check in StakedEXA::harvest will cause a DoS for new depositors as the function will revert whenever the amount deposited back into the market (savings) is low enough to not mint any share.

Root Cause

In StakedEXA:358 there is a missing check that will cause a DoS for new depositors whenever the amount deposited back into the market (savings) is low enough to not mint any share.

https://github.com/sherlock-audit/2024-07-exactly-stacking-contracts/blob/main/protocol/contracts/StakedEXA.sol#L358

Internal pre-conditions

1. The save amount calculated in harvest must be low enough to not mint any share when deposited back into the market. When the market has been active for some time, it's usual that the exchange rate is quite high, causing a deposit of a few wei not to mint any share at all.

External pre-conditions

1. The amount that we'll withdraw and deposit back on harvest directly depends on the treasury fees accrued on the underlying market. For this DoS to be persistent, the market specified in StakedEXA must have low activity so that the treasury fees accrued are a low amount.

Attack Path

1. Any user calls deposit or mint to stake EXA tokens.
2. The deposit or mint function calls the internal _update function before minting the actual shares.
3. The _update function calls harvest to distribute the dividends from the provider market.
4. The harvest function withdraws from the market all funds from the treasury address (provider).
5. The save amount is calculated based on the providerRatio and that amount of tokens are deposited back into the market.
   • When the save amount is low enough, the shares minted in the market will be 0, causing a revert on this line.

As long as the provider market is quite inactive and the treasury fees accrued are a low amount, the harvest function will revert every time causing a DoS on all deposits in StakedEXA. If the activity in the market stays low for quite some time, the DoS can be extended to more than 7 days, making this issue valid given that staking is a core protocol functionality.

Impact

All stakers will experience a DoS on new deposits within the StakedEXA contract.

PoC

No response

Mitigation

To mitigate this issue is recommended to check within the harvest function if the amount to deposit back into the market will mint any share at all:

  function harvest() public whenNotPaused {
    // ...

    memMarket.withdraw(assets, address(this), memProvider);
    uint256 save = assets - amount;
+   uint256 sharesMinted = market.previewDeposit(save);
+   if (save != 0 && sharesMinted != 0) memMarket.deposit(save, savings);
-   if (save != 0) memMarket.deposit(save, savings);

    // ...
  }

[santiellena]

Escalate.

This issue deserves Low severity and not Medium.
The point made in the step 5 on the Attack Path will never occur due to a previous condition not mentioned in the issue.
The condition: if (duration == 0 || amount < rewards[providerAsset].duration) return;

In the context in the harvest function:

    uint256 assets = Math.min(
      memMarket.convertToAssets(memMarket.allowance(memProvider, address(this))),
      memMarket.maxWithdraw(memProvider)
    );
    uint256 amount = assets.mulWadDown(providerRatio);
    IERC20 providerAsset = IERC20(address(memMarket.asset()));
    uint256 duration = rewards[providerAsset].duration;
    if (duration == 0 || amount < rewards[providerAsset].duration) return;


    memMarket.withdraw(assets, address(this), memProvider);
    uint256 save = assets - amount;
    if (save != 0) memMarket.deposit(save, savings);

The amount variable is calculated as a proportion of assets, which is expected to be a smaller proportion. Then, the previously mentioned condition is checked on that smaller amount.

The smaller value that amount can take to reach the line that could potentially revert is duration + 1.
As example, given a duration = 12 weeks = 31536000 (in tests is 24 weeks), if amount >= 31536000 + 1 and the assets variable which depends on the providerRatio value (in tests is 0.1e18) is (31536000 + 1) * 9 = 283824009, the save variable would be 283824009 - (31536000 + 1) = 252288008.

Therefore, the smaller value that the save variable can take is 252288008 which will only revert in a far future in case the market ratio is extremely inflated.

Summing up, there is already an existing check to prevent the described situation in this report which makes this issue Low/Invalid. However, the recommendation of code changes must be done in order to prevent unexpected situations.

[sherlock-admin3]

Escalate.

This issue deserves Low severity and not Medium.
The point made in the step 5 on the Attack Path will never occur due to a previous condition not mentioned in the issue.
The condition: if (duration == 0 || amount < rewards[providerAsset].duration) return;

In the context in the harvest function:

    uint256 assets = Math.min(
      memMarket.convertToAssets(memMarket.allowance(memProvider, address(this))),
      memMarket.maxWithdraw(memProvider)
    );
    uint256 amount = assets.mulWadDown(providerRatio);
    IERC20 providerAsset = IERC20(address(memMarket.asset()));
    uint256 duration = rewards[providerAsset].duration;
    if (duration == 0 || amount < rewards[providerAsset].duration) return;


    memMarket.withdraw(assets, address(this), memProvider);
    uint256 save = assets - amount;
    if (save != 0) memMarket.deposit(save, savings);

The amount variable is calculated as a proportion of assets, which is expected to be a smaller proportion. Then, the previously mentioned condition is checked on that smaller amount.

The smaller value that amount can take to reach the line that could potentially revert is duration + 1.
As example, given a duration = 12 weeks = 31536000 (in tests is 24 weeks), if amount >= 31536000 + 1 and the assets variable which depends on the providerRatio value (in tests is 0.1e18) is (31536000 + 1) * 9 = 283824009, the save variable would be 283824009 - (31536000 + 1) = 252288008.

Therefore, the smaller value that the save variable can take is 252288008 which will only revert in a far future in case the market ratio is extremely inflated.

Summing up, there is already an existing check to prevent the described situation in this report which makes this issue Low/Invalid. However, the recommendation of code changes must be done in order to prevent unexpected situations.

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.

[santipu03]

The escalation is missing the fact that the values for duration and providerRatio can be far different than the ones shown in the tests according to the README:

Are there any limitations on values set by admins (or other roles) in the codebase, including restrictions on array lengths?
no

When the value of duration is lower than 12 weeks and the providerRatio is higher than 0.1e18, the bug will happen more often, therefore causing the DoS on deposits consistently.

[santiellena]

I understand your point of view, however, I don't think we can consider issues that depend on administrators setting parameters with nonsensical values.

Doing the math again with shorter periods and a really high providerRatio:

• duration = 4 weeks = 2419200
• providerRatio = 0.4e18 (cannot be 0.5e18 >=)
  Then, the minimum value that amount can take is 2419200 + 1. assets amount will be equal to 6048002. Therefore, save = 6048002 - 2419201 = 3628801.

Still in the case the configuration makes save the smallest possible amount, the statement of the escalation remains valid.

[WangSecurity]

I agree with the escalation. The issue is dependent on admin values and as I understand, with a specific set of values this issue can never happen or the admin can change both duration and provider ratio to mitigate the DOS. Additionally, it requires a high exchange rate on a not active pool, which seems odd, I understand this may be possible and I'm mistaken here, but still, the admin can mitigate the issue within 7 days.

Planning to accept the escalation and invalidate the issue.

[WangSecurity]

Result:
Invalid
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• santiellena: accepted

[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.