search

sherlock-audit / 2024-04-arcadia-pricing-module-judging

4 stars 4 forks source link

Bauchibred - Restricted Aeroguage admin could cause core functionalities in protocol to be unavailable #25

Closed sherlock-admin3 closed 7 months ago

sherlock-admin3 commented 7 months ago

Bauchibred

medium

Restricted Aeroguage admin could cause core functionalities in protocol to be unavailable

Summary

Restricted Aeroguage admin could cause core functionalities in protocol to be Dos'd

Vulnerability Detail

Take a look at https://github.com/sherlock-audit/2024-04-arcadia-pricing-module/blob/54832626b806dcb72e250935fc193acb95e0cf8a/accounts-v2/src/asset-modules/Aerodrome-Finance/StakedAerodromeAM.sol#L88-L100

    function _stakeAndClaim(address asset, uint256 amount) internal override {
        address gauge = assetToGauge[asset];

        // Claim rewards
        IAeroGauge(gauge).getReward(address(this));

        // Stake asset
        ERC20(asset).approve(gauge, amount);
        IAeroGauge(gauge).deposit(amount);
    }

Now consider this section https://github.com/sherlock-audit/2024-04-arcadia-pricing-module/blob/54832626b806dcb72e250935fc193acb95e0cf8a/README.md#L40-L44

### Q: Are the admins of the protocols your contracts integrate with (if any) TRUSTED or RESTRICTED? If these integrations are trusted, should auditors also assume they are always responsive, for example, are oracles trusted to provide non-stale information, or VRF providers to respond within a designated timeframe?

(...)
Gauge.sol:

- Owner should ensure the balance of reward token of the contract is sufficient.
- Worst case anyone can top up the balance if owner is non-responsive
- Apart from the mentioned top-up issue RESTRICTED
  (...)

Evidently, we can see that asides top-up issue the Guage.sol admin is restricted and not trusted, navigating to the integrated Guage.sol contract here https://github.com/aerodrome-finance/contracts/blob/b934e7ae398ea6c251a4d5af2119776c06f1f23d/contracts/gauges/Gauge.sol

    /// @inheritdoc IGauge
    function deposit(uint256 _amount) external {
        _depositFor(_amount, _msgSender());
    }

    /// @inheritdoc IGauge
    function deposit(uint256 _amount, address _recipient) external {
        _depositFor(_amount, _recipient);
    }

    function _depositFor(uint256 _amount, address _recipient) internal nonReentrant {
        if (_amount == 0) revert ZeroAmount();
        if (!IVoter(voter).isAlive(address(this))) revert NotAlive();

        address sender = _msgSender();
        _updateRewards(_recipient);

        IERC20(stakingToken).safeTransferFrom(sender, address(this), _amount);
        totalSupply += _amount;
        balanceOf[_recipient] += _amount;

        emit Deposit(sender, _recipient, _amount);
    }

We can see that whenever depositing, the attempt queries to check if the guage has been killed or not, i.e if (!IVoter(voter).isAlive(address(this))) revert NotAlive(); , considering the guage can be killed by the admin, all attempts at staking and claiming via the StakingAm.sol would now fail, keep in mind that this function is essentially used whenever minting a new position or increasing liquidity.

Impact

RESTRICTED external admins can break core functionality in protocol, since staking/claiming/minting and increasing liquidity attempts would always encounter a DOS.

Code Snippet

https://github.com/sherlock-audit/2024-04-arcadia-pricing-module/blob/54832626b806dcb72e250935fc193acb95e0cf8a/accounts-v2/src/asset-modules/Aerodrome-Finance/StakedAerodromeAM.sol#L88-L100

Tool used

Manual Review

Recommendation

Ensure that the protocol team and its users are aware of the risks of such an event and develop a contingency plan to manage it!

sherlock-admin4 commented 7 months ago

2 comment(s) were left on this issue during the judging contest.

takarez commented:

given the readMe, this should be valid medium; medium(5)

shealtielanz commented:

killing gauge will lead to issues

Thomas-Smets commented 7 months ago

Invalid, if the gauge is killed only minting and increasing Liquidity will revert. Existing positions can still claim rewards and close their positions.

Since there are no rewards anymore there is no point in minting or increasing liquidity. We could check if a gauge is killed and revert but this is exactly the same what happens now: that minting/increasing liquidity of killed gauges reverts.

ZanyBonzy commented 7 months ago

Escalate

Hi, On sherlock, to acertain the validity of an issue, there are two major effects to be taken into consideration.

  1. Loss of funds;
  2. Breaking core contract functionality;

This report and its duplicate point out how killed gauges, causes adding and increasing liquidity to become impossible as pointed out in #29 , dossing a core contract functionality. While users can still remove liquidity, hence funds are still safe, as liquidity cannot be added to the pool, the purpose of the pool/contract is defeated, sooner or later rendering it unusable and "useless".

Seeing as this is due to the actions of a "Restricted admin", as pointed out in the readme, this and its duplicate, therefore should be valid issues. Note that issues about killed gauges was not listed as a known issue, nor pointed out in readme, nor was it stated that external admins pausing integrations acceptable.

Are the admins of the protocols your contracts integrate with (if any) TRUSTED or RESTRICTED? If these integrations are trusted, should auditors also assume they are always responsive, for example, are oracles trusted to provide non-stale information, or VRF providers to respond within a designated timeframe?

The protocols we integrate with are:

...

Gauge.sol:

Owner should ensure the balance of reward token of the contract is sufficient. Worst case anyone can top up the balance if owner is non-responsive Apart from the mentioned top-up issue RESTRICTED

sherlock-admin3 commented 7 months ago

Escalate

Hi, On sherlock, to acertain the validity of an issue, there are two major effects to be taken into consideration.

  1. Loss of funds;
  2. Breaking core contract functionality;

This report and its duplicate point out how killed gauges, causes adding and increasing liquidity to become impossible as pointed out in #29 , dossing a core contract functionality. While users can still remove liquidity, hence funds are still safe, as liquidity cannot be added to the pool, the purpose of the pool/contract is defeated, sooner or later rendering it unusable and "useless".

Seeing as this is due to the actions of a "Restricted admin", as pointed out in the readme, this and its duplicate, therefore should be valid issues. Note that issues about killed gauges was not listed as a known issue, nor pointed out in readme, nor was it stated that external admins pausing integrations acceptable.

Are the admins of the protocols your contracts integrate with (if any) TRUSTED or RESTRICTED? If these integrations are trusted, should auditors also assume they are always responsive, for example, are oracles trusted to provide non-stale information, or VRF providers to respond within a designated timeframe?

The protocols we integrate with are:

...

Gauge.sol:

Owner should ensure the balance of reward token of the contract is sufficient. Worst case anyone can top up the balance if owner is non-responsive Apart from the mentioned top-up issue RESTRICTED

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.

Thomas-Smets commented 7 months ago

As liquidity cannot be added to the pool, the purpose of the pool/contract is defeated, rendering it "useless".

This is incorrect, you can still add funds to the Aerodrome pool, just not stake it in the StakedAerodromeAM. But nothing prevents you from adding funds to the pool via the WrappedAerodromeAM. Now you will earn trading fees instead of reward emissions as yield.

The purpose of the pool/contract is defeated, rendering it "useless".

When the emissions stop, that staked pool actually does become "useless", since it no longer earns yield. It is just part of the lifecycle of a gauge. Since I don't see it as an issue that users cannot deposit in killed gauges it is indeed not listed as a known issue, since we do not consider it an issue. I would even say the opposite, if we would have in a way allowed users to mint position of killed gauges that would be an issue.

Recommendation Ensure that the protocol team and its users are aware of the risks of such an event and develop a contingency plan to manage it!

If I look at your recommendation, can you please elaborate:

In my opinion our contingency plan is that for killed gauges, users cannot create/increase positions, only decrease them. Which is already nicely handles by the Aerodrome contract. I do not see the purpose if reverting in our contract since that just results in redundant checks and contract calls.

Bauchibred commented 7 months ago

This is incorrect, you can still add funds to the Aerodrome pool, just not stake it in the StakedAerodromeAM. But nothing prevents you from adding funds to the pool via the WrappedAerodromeAM. Now you will earn trading fees instead of reward emissions as yield.

This is a confirmation that the StakedAerodromeAM would infact no longer function as expected, whereas I see your POV about disputing the issue, I believe this does not hold ground when considering the validity on Sherlock, navigating to the section 5 of the sherlock docs, i.e how to identify a medium issue, it's clearly indicated that issues that showcases that a core contract functionality is broken should be finalized as a medium finding, now all parties would agree that the core purpose/functionality of the StakedAerodromeAM contract is infact for users to be able to stake and then earn yield for their stake which as this report showcases would be impossible, asides this fact.

In regards to your confusion about the recommendation provided, this is due to the nature of the issue, this is realistically the only fix one can suggest about an issue from a restricted admin as you are not in control which is to ensure the users are in the know that this could happen, I'd employ you check historical issues that have stemmed from the fact that external admins are restricted, we can see that the idea of the fixes are similar, as realistically the bug case is from the external admin who is not considered trusted and the only thing to do is to relay the informations to the users to tell them that there is a possibility where staking/getting yield from their stakes would no longer be possible due to the external integration.

TLDR: The grounds of disputing this issue by the sponsor is not enough reason to invalidate it as per Sherlock's rules on external admin integrations being restricted which IMO places this issue as valid... quoting another section of Sherlock's judging docs below:

III. Sherlock's standards#5.2: When external-admin=restricted, issues related to these external admins affecting a protocol (being audited) by updating the external protocol parameters is a valid issue (Example: Aave governance has the intention to improve the Aave protocol) as the bug can occur even when the external admin is well intended

Thomas-Smets commented 7 months ago

It is not a bug, it functions exactly as intended. When a gauge is killed no more deposits are allowed.

Hash01011122 commented 7 months ago

Agreed, with the points mentioned by @Thomas-Smets do you want to add anything @Bauchibred, @ZanyBonzy?

ZanyBonzy commented 7 months ago

Hi @Hash01011122, thanks for the good work.

  1. The sponsor claims its part of a lifecycle of a gauge which is fair enough, but the issue here is the "Restricted Admin" meaning the admins can unintentionally/"maliciously" shut down the gauge at any time they want, with/without prior warning.

  2. As per the hierarchy of truth, the readme overrides everything else, and information about this wasn't introduced in the readme. I'd probably go on a limb and say the sponsor wasn't aware of a possibility of this happening before we pointed this out. Just like users will most likely not be aware of the source of the issue from if they suddenly find out that they can't deposit or increase liquidity. It's the reason for my recommendation, "include information about this in the docs"

  3. "Core functionalities must be broken, rendering the contract useless" as a criteria for medium severity issue. Due to the gauge being killed, two core functionalies are blocked, and by the end of it, the purpose of the contract is defeated, rendering it useless, as such, users looking to earn rewards through staking will not be able to.

  4. Also, if we want to discuss possible loss of funds, killing a gauge removes the rewards it is entitled to, so all rewards accumulated by the pool in the gauge for the time period is also lost when the gauge is killed. I pointed that out in #29

Ultimately, this is a case of following the readme and judging guidelines, these issues meet at least one of needed criteria for a medium severity issue if not both, as core functionality is dossed which potentially renders the contract useless, and accumulated rewards during a timeframe is also lost(i.e potential loss of funds). I believe, the rules should be apply here for consistency, as we can't just choose to follow them on the basis of whether the sponsors agree or not.

That's all I have to say, i'll leave the rest to @Bauchibred as I'm interested in hearing what he thinks.

Bauchibred commented 7 months ago

Thanks for the re-review @Hash01011122, I don't think there is a need for any new information based on the discussion already had from all parties on this issue, considering this finalising comment made by @ZanyBonzy, as I previously stated in this discussion and since this is an audit contest, the sponsor is in his right to "dispute" this finding or any other, however that is not a deciding line on it's validity on Sherlock, and based on the judging guidelines and historical data I believe this finding should be reassigned a valid state.

shealtielanz commented 7 months ago

I agree with @Bauchibred on this issue, I think @ZanyBonzy did a good job to further dissect why this should be a valid finding

Thomas-Smets commented 7 months ago
  1. It is part of the lifecycle of every gauge, every Gauge can and will be shut down at some point, I cannot even believe that this a even a discussion. And this is handled by the contract correctly! It is not because something reverts that the contract is broken. The contract does not revert when is should not revert, and the contract does revert when it should revert.
  2. You indeed go on a limb since we were very aware. Since we don't see this as an issue at all. Our users understand that when a gauge is killed they no longer can earn rewards through staking. I would even go further, I think our users would be less happy if they accidentally increase liquidity for a positions that no longer earns rewards.
  3. The contract works just fine, it allows users to earn yield when rewards are enabled, and it does block increasing positions when rewards no longer are distributed. In the latter case the contract is not useless, all rewards distributed to the Gauge are still claimable by the existing positions.
  4. Killing gauges has no impact on any of the rewards accumulated by the pool in the gauge (which we account for in the Asset Module). It only has an effect of the rewards accumulated by the gauge in the voter (which we do not account for in the Asset Module!)

This whole discussion boils down to the fact if we can call reverting of minting positions/increasing liquidity after a Gauge was killed a "Breaking of core contract functionality".

And in my opinion that is a no, after a gauge is killed, the underlying state of a contract we integrate with is changed. And our contract handles both states well:

Hash01011122 commented 7 months ago

Do you want to add anything @ZanyBonzy @Bauchibred??

cvetanovv commented 7 months ago

This is issue is invalid. A sponsor correctly shows how things work as they should. In the escalation, Watson suggests that this issue met the criteria for Medium:

  1. Loss of funds;
  2. Breaking core contract functionality;

I plan to reject the escalation and leave the issue as it is.

Evert0x commented 6 months ago

Result: Invalid Unique

sherlock-admin4 commented 6 months ago

Escalations have been resolved successfully!

Escalation status: