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

4 stars 4 forks source link

0x73696d616f - `SlipstreamAM::_getFeeAmounts()` fetches the liquidity of the position, allowing the owner to increase `USD` balance as it pleases #31

Closed sherlock-admin3 closed 6 months ago

sherlock-admin3 commented 6 months ago

0x73696d616f

medium

SlipstreamAM::_getFeeAmounts() fetches the liquidity of the position, allowing the owner to increase USD balance as it pleases

Summary

SlipstreamAM::_getFeeAmounts() fetches the liquidity of position, instead of using the cached liquidity. Thus, users that have pending fees may increase the liquidity of their positions to instantly boost the USD value of their position, as it is pro-rata to liquidity.

Vulnerability Detail

SlipstreamAM::_getFeeAmounts() gets the fees of a position using the actual liquidity of the position in the pool. This is not the case for other asset moduels such as WrappedAerodromeAM, which uses the cached totalWrapped. Due to the fact that fees are calculated pro-rata to liquidity, this means that anyone may increase the USD value of an already deposited assetId, which can serve several different purposes within the protocol and may be used to game it.

Impact

Incosistent and silent increase of the USD value of an account up to principal0 and principal1 which may be used to game the protocol.

Code Snippet

SlipstreamAM::_getFeeAmounts()` fetching the actual liquidity of a position instead of the cached one.

    ...
    function _getFeeAmounts(uint256 id) internal view returns (uint256 amount0, uint256 amount1) {
        (
            ,
            ,
            address token0,
            address token1,
            int24 tickSpacing,
            int24 tickLower,
            int24 tickUpper,
            uint256 liquidity, // gas: cheaper to use uint256 instead of uint128. //@audit here
            uint256 feeGrowthInside0LastX128,
            uint256 feeGrowthInside1LastX128,
            uint256 tokensOwed0, // gas: cheaper to use uint256 instead of uint128.
            uint256 tokensOwed1 // gas: cheaper to use uint256 instead of uint128.
        ) = NON_FUNGIBLE_POSITION_MANAGER.positions(id);
    ...

Tool used

Manual Review

Vscode

Recommendation

Use the cached liquidity to prevent abuse from accounts.

function _getFeeAmounts(uint256 id) internal view returns (uint256 amount0, uint256 amount1) {
    (
        ,
        ,
        address token0,
        address token1,
        int24 tickSpacing,
        int24 tickLower,
        int24 tickUpper,
        , // unnecessary
        uint256 feeGrowthInside0LastX128,
        uint256 feeGrowthInside1LastX128,
        uint256 tokensOwed0, // gas: cheaper to use uint256 instead of uint128.
        uint256 tokensOwed1 // gas: cheaper to use uint256 instead of uint128.
    ) = NON_FUNGIBLE_POSITION_MANAGER.positions(id);

    uint256 liquidity = uint128(assetToLiquidity[assetId]); // get liquidity
    ...
}
Thomas-Smets commented 6 months ago

Invalid.

Increasing the liquidity of an already deposited LP position does not increase the fee amounts. See the Slipstream code: https://github.com/aerodrome-finance/slipstream/blob/3e8add8d08110d4eb563fa17ab7fa63b1b79d634/contracts/periphery/NonfungiblePositionManager.sol#L255

The fee amounts collected before the increase are stored as tokensOwed, and the position.feeGrowthInside0LastX128 and position.feeGrowthInside1LastX128 are reset.

image

This approach is by the way also used in the previously audited UniV3AM.

Hash01011122 commented 6 months ago

Thanks for your input invalidating this issue

0x73696d616f commented 6 months ago

Escalate

The sponsor is correct that the fees will not be multiplied by the liquidity after liquidity has been increased, as the current fees would be carried over to positions.tokenOwed0 and positions.tokenOwed1. Unfortunately I realized this after the contest had ended and could not edit the submission. This is a slight incorrection that does not invalidate the root cause.

The issue should stand due to the fact that after adding liquidity, the fees will now accumulate pro-rata to the postion's liquidity in the pool, not the cached one, opposed to the behaviour of WrappedAerodromeAM.

This means that users can still increase their USD balance this way by accumulating much more fees than supposed, and possibly bypass the exposure limits.

If the user adds liquidity with a very significant amount, fees would accrue much faster, having the same impact as the original submission, but in a slightly longer time frame.

sherlock-admin3 commented 6 months ago

Escalate

The sponsor is correct that the fees will not be multiplied by the liquidity after liquidity has been increased, as the current fees would be carried over to positions.tokenOwed0 and positions.tokenOwed1. Unfortunately I realized this after the contest had ended and could not edit the submission. This is a slight incorrection that does not invalidate the root cause.

The issue should stand due to the fact that after adding liquidity, the fees will now accumulate pro-rata to the postion's liquidity in the pool, not the cached one, opposed to the behaviour of WrappedAerodromeAM.

This means that users can still increase their USD balance this way by accumulating much more fees than supposed, and possibly bypass the exposure limits.

If the user adds liquidity with a very significant amount, fees would accrue much faster, having the same impact as the original submission, but in a slightly longer time frame.

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 6 months ago

The fees will now accumulate pro-rata to the postion's liquidity in the pool, not the cached one, opposed to the behaviour of WrappedAerodromeAM.

You cannot really compare with the WrappedAerodromeAM.sol, those are fungible tokens while the SlipstreamAM are non-fungible positions. A better comparison is with UniV3AM.sol.

This` means that users can still increase their USD balance this way by accumulating much more fees than supposed, and possibly bypass the exposure limits.

It is not a way the user can instantly manipulate exposure, those fees actually have to accrue.

If the user adds liquidity with a very significant amount, fees would accrue much faster, having the same impact as the original submission, but in a slightly longer time frame.

That fee accurel causes exposures to exceed the exposure cap is something that cannot be fully prevented (since checks can only be done on a deposit). For that exact reason the maximum amount that fees count as collateral for the SlipstreamAM is limited by the principle amount: https://github.com/arcadia-finance/accounts-v2/blob/35634c7bb1e4c43db21c199ff10bec219ae16160/src/asset-modules/Slipstream/SlipstreamAM.sol#L222

0x73696d616f commented 6 months ago

Agreed that the perfect fix is not possible, but there is an option to protect the protocol.

Due to the fact that fees go to tokenOwed for every addLiquidity() call, it's not enough to multiply the current fees by the cached liquidity here.

In addition to this, positions.tokenOwed0 and positions.tokenOwed1 should be multiplied by the cached liquidity and divided by the current liquidity to get the equivalent cached liquidity tokens owed. This will ensure that users that add liquidity post their deposit will always get less or equal fees to their SlipstreamAM position, protecting the protocol.

The only drawback is that if a user adds liquidity to their position after some fees have accrued, the fees earned while the liquidity was the cached one will also be multiplied by the cached / current liquidity ratio, decreasing their actual fees. However, this should not be a problem for honest users as they're not supposed to add liquidity to their position anyway.

Hash01011122 commented 6 months ago

Thing I want to point out:

Unfortunately I realized this after the contest had ended and could not edit the submission. This is a slight incorrection that does not invalidate the root cause.

After mentioning this do you think your escalation should be accepted @0x73696d616f?

0x73696d616f commented 6 months ago

Yes, the root cause is correct, the only difference is that fees would take longer to accumalate. The sponsor has confirmed that fees would increase more than supposed.

Thomas-Smets commented 6 months ago

No I didn't confirm that the root cause is correct. What I said is that fees can exceed caps for every derived asset module (and that is not directly an issue). And for that we already have a mitigation in place here, that total fees are capped.

0x73696d616f commented 6 months ago

It's a fact that fees will accrue more if a user adds liquidity to the pool. This could be used to indirectly increase the balance. I mentioned a recommendation that would make this attack impossible.

Thomas-Smets commented 6 months ago

We already mitigated it by capping it.

0x73696d616f commented 6 months ago

Yes but not completely, as I am pointing out.

Thomas-Smets commented 6 months ago

Yes, the cap is on the final fee amounts. It is independent if the cashed or actual liquidity is used.

0x73696d616f commented 6 months ago

But it can be reached much faster. I do not understand why we are still commenting on this issue, the issue exists and the fix is not complete.

Thomas-Smets commented 6 months ago

What does that matter? It is capped and cannot be instantly be manipulated.

It is not a valid issue

0x73696d616f commented 6 months ago

You can just wait a few blocks. This is intellectual dishonesty at this point.

Thomas-Smets commented 6 months ago

You can maximally have an exposure of 2x the principal. Cashing the liquidity or not has no impact on it, and the fees still actually have to accrue over time. That it takes a bit longer or shorter is not an additional issue. Main point is that it is already capped!!

Can you point according to which Sherloch rule this a valid medium then?

Thomas-Smets commented 6 months ago

That it can be reached faster (but still not instant) is not an issue

0x73696d616f commented 6 months ago

Glad you accept that fees accrual can be gamed.

How is it not an issue if given a starting point A, at point B, a user would have much more balance due to fees accrual than a regular user that did not increase its liquidity. Effectively, security wise a user could exploit this and increase its balance at point B, having an edge over honest users. Also, the mentioned fix would completely mitigate this exploit, why would you not do it?

j-vp commented 6 months ago

The core of this finding is that a user can reach a foreseen and accepted state faster by some workaround.

However, reaching such an accepted state faster than others is not "unfair" to anyone, nor does it lead to funds at risk. Instead, the smart contracts are foreseen to handle such cases (by limiting the fee to equal the principal). Keep in mind that this is not related to getting rewards etc, but really relates to the value of the position. If they can increase it faster, it means their position is also worth more.

The implementation of the current smart contract thus mitigate a potential exploit/additional risk on this vector, and handle this case as designed (as in; how it is intended and written in the contracts).

Per Sherlock rules, a proposed "better" implementation of a certain functionality does not make it a valid issue.

-> as intended, no funds at risk -> no issue.

Thomas-Smets commented 6 months ago

Jesus Christ, he cannot have a much higher balance since the total is capped! Stop ignoring this.

The balance of the user is real by the way. The only reason that we cap it (and why we for the principal use a cashed liquidity) is to have a way to have a max exposure to certain assets and protocols. That a max exposure is reached faster is not an issue. If a max exposure could easily be far exceeded would be a problem.

In this case using the actual liquidity does not lead an increase of how far the max exposure can be exceeded, just a bit faster, which as said is Not an issue. It is not an exploit.

j-vp commented 6 months ago

To clarify: if the user deposits a position of 1 USD, then adds one gazillion more to the liquidity, the maximum the position will be valued at (and will count towards the exposure) will be 2 USD after earning 1 USD (or any amount more) in fees.

0x73696d616f commented 6 months ago

The core of this finding is that a user can reach a foreseen and accepted state faster by some workaround.

This can not be a valid argument as it would invalidate most issues. This workaround is a security concern, users are not supposed to use unconventional, undocumented techniques for their advantage. This is the definition of an exploit.

However, reaching such an accepted state faster than others is not "unfair" to anyone, nor does it lead to funds at risk. Instead, the smart contracts are foreseen to handle such cases (by limiting the fee to equal the principal). Keep in mind that this is not related to getting rewards etc, but really relates to the value of the position. If they can increase it faster, it means their position is also worth more.

It's unfair as in shady users will have this edge over honest users. They can at any point increase their balance up to 2x more faster than their deposit intended to. Let's say 2 users deposit the same amount, but user 2 exploits this and adds liquidity. One of the users now has a much bigger position, but they started at the same point.

The implementation of the current smart contract thus mitigate a potential exploit/additional risk on this vector, and handle this case as designed (as in; how it is intended and written in the contracts).

It mitigates the fact that the balance can not be infinitely increased, but not that it can be increased by a workaround.

Jesus Christ, he cannot have a much higher balance since the total is capped!

Having 2x the balance than a regular user that did the same deposit at some point can be considered 'much'.

To clarify: if the user deposits a position of 1 USD, then adds one gazillion more to the liquidity, the maximum the position will be valued at (and will count towards the exposure) will be 2 USD after earning 1 USD (or any amount more) in fees.

If the numbers are replaced by 100_000 USD it becomes impactful.

In all honesty if this attack vector is compared to issue #18, it's much easier and less costly to perform. However, thankfully this one may be completely mitigated.

All in all, I have proved that fees accrual can be gamed, sponsors have confirmed and a fix was given that completely mitigates the attack.

j-vp commented 6 months ago

It's not an exploit as you mentioned, it's just another way of arriving at the same (accepted and expected) endstate. The whole beauty of smart contracts is that multiple ways can be used to reach a given state, however there should be no way to circumvent that accepted state. Any way to reach an accepted state is an accepted way of doing it. Any way to reach an unaccepted or unintended state is an exploit. In this case, the latter is not applicable.

There is no "shady" user in this case, I believe you have the idea that by adding more liquidity you get some kind of additional bonus or reward or that your position gets overvalued or some kind. It's not, your position is just worth more. The additional exposure used by that "shady" user is actual value of the collateral!

I would even say it's already "unfair" to the "shady" user that he can only use a fraction of the actual market value of his position as collateral, and not the other way around. User 1 deposits 1000 USD value and gets 1 USD fees per day. User 2 deposits 1000 USD, then adds 1m USD and gets 1000 USD fees per day. The second user now has 999000 USD value stuck to have no benefit whatsoever.

Keep in mind that to get an additional 100k USD as in your example, your position size must be many, many times more, and this is idle capital sitting around for a long time until the whole 100k USD is earned in fees (remember that the additional 100k must first be earned in fees, and is thus actual value!)

0x73696d616f commented 6 months ago

It's not an exploit as you mentioned, it's just another way of arriving at the same (accepted and expected) endstate. The whole beauty of smart contracts is that multiple ways can be used to reach a given state, however there should be no way to circumvent that accepted state. Any way to reach an accepted state is an accepted way of doing it. Any way to reach an unaccepted or unintended state is an exploit. In this case, the latter is not applicable.

This is false. Let's say the user has reached the exposure cap. If a user manages to increase its balance by doing some external interaction, it is an exploit.

There is no "shady" user in this case, I believe you have the idea that by adding more liquidity you get some kind of additional bonus or reward or that your position gets overvalued or some kind. It's not, your position is just worth more. The additional exposure used by that "shady" user is actual value of the collateral!

I would even say it's already "unfair" to the "shady" user that he can only use a fraction of the actual market value of his position as collateral, and not the other way around. User 1 deposits 1000 USD value and gets 1 USD fees per day. User 2 deposits 1000 USD, then adds 1m USD and gets 1000 USD fees per day. The second user now has 999000 USD value stuck to have no benefit whatsoever.

What I am saying is that the user increases its balance at some point B without interacting with the protocol and going through all the checks, such as the exposure cap.

Keep in mind that to get an additional 100k USD as in your example, your position size must be many, many times more, and this is idle capital sitting around for a long time until the whole 100k USD is earned in fees (remember that the additional 100k must first be earned in fees, and is thus actual value!)

This depends on market conditions, not necessarily much more. Fees are based on swaps, not time.

0x73696d616f commented 6 months ago

This is what I am saying

If a max exposure could easily be far exceeded would be a problem.

It is a problem, if the exposure cap is 1 million, and the user deposits a total of 1 million, goes to a pool, increases liquidity and gets a huge amount of fees, the position could easily reach 2 million in a reasonable time frame, depending on market activity. The liquidity would not even be idle as it is accruing fees.

j-vp commented 6 months ago

But the maxExposure's already take this into account, so what is negative towards the protocol?

0x73696d616f commented 6 months ago

what do you mean?

0x73696d616f commented 6 months ago

It takes into account up to 2x, until there it may be gamed.

j-vp commented 6 months ago

This is what I am saying

If a max exposure could easily be far exceeded would be a problem.

It is a problem, if the exposure cap is 1 million, and the user deposits a total of 1 million, goes to a pool, increases liquidity and gets a huge amount of fees, the position could easily reach 2 million in a reasonable time frame, depending on market activity. The liquidity would not even be idle as it is accruing fees.

Earning 1m in fees in a single pool in a short timeframe is not really realistic... And at this point a user has a large position at risk of potential liquidation, which will be sold for the open debt value, far below market value in this case.

0x73696d616f commented 6 months ago

Earning 1m in fees in a single pool in a short timeframe is not really realistic...

Depends on market conditions and the deposits of a user. It is still a way to surpass it.

And at this point a user has a large position at risk of potential liquidation, which will be sold for the open debt value, far below market value in this case.

It does not have to be at risk of a potential liquidation, or the risk may be very very low. Additionally, the user is accruing fees on its liquidity, it incurrs no extra cost.

This way of increasing the usd balance is much easier and general than #18, it should be valid too.

j-vp commented 6 months ago

It takes into account up to 2x, until there it may be gamed.

Again, so? What's the negative part here? It's like saying "the lending pool can be filled up quick", that's what it's for!

j-vp commented 6 months ago

Look at it as if the 1x is for principal, the 2x is for fees. Doesn't matter who filles it up, how it is filled up or how fast it is filled up.

Then just look at the 2x in total; doesn't matter how/who fills it up

0x73696d616f commented 6 months ago

The exposure cap is being circumvented up to 2x with no cost. This is an exploit, no matter how we phrase it.

0x73696d616f commented 6 months ago

Look at it as if the 1x is for principal, the 2x is for fees. Doesn't matter who filles it up, how it is filled up or how fast it is filled up. Then just look at the 2x in total; doesn't matter how/who fills it up

This is just a simplistic view which does not tell the whole story. Take 2 snapshots in time, user who adds liquidity outside the protocol will have increased its position significantly at time B by abusing fees, with no cost.

0x73696d616f commented 6 months ago

2 users have exactly the same amount of funds, both deposit liquidity until the exposure cap while having some leftover liquidity. User 1 realizes it can not deposit any more and allocates the liquidity elsewhere, as intended. User 2 realizes it can increase its balance, bypass the exposure cap significantly and still accrue fees and increases the liquidity in its position maliciously. How is this fair for user 1? How is this fair for the protocol if a user can easily bypass the exposure cap?

Thomas-Smets commented 6 months ago

To use your last example.

At the moment user 2 increases his liquidity, his balance will not increase at all. That's what I already told you in my first reply.

From that moment on, both users will start earning fees on their position. User2 earns fees faster, but as we already said multiple times. Speed does not matter and is not an issue

At some point fees of user2 will equal principal. And additional fees do not count anymore.

At another point in time fees for user1 will also equal principal. Both users balance is now exactly the same. No difference for the protocol between user1 and user2 (you could even argue that user2 is at a slight disadvantage since they have now deposited more liquidity fir a smaller balance hence are less capital efficient as user1).

Nothing is not circumvented, nothing is exploited, nowhere does it lead to loss of funds for any user or the protocol, the final exposure towards the protocol is equal.

Thomas-Smets commented 6 months ago

And also it is not at no cost. User2 actually has to add the liquidity.

Say both start with a properly deposited 100k in liquidity.

If user2 wants to earn fees double as fast, he actually has to deposit another 100k, but his balance stays 100k!

Both the balance of user1 and user2 can grow from their properly deposited 100k to maximally 200k. That user2 improperly deposited another 100k in the lp position does not change that.

That 100k IS a real cost with no direct or long term benefit. Only affects the speed at which fees accumulate. (Which is not an exploit, user 2 could also have deposited in a pool with twice the trading volume and then fees would also accrue twice as fast).

0x73696d616f commented 6 months ago

At the moment user 2 increases his liquidity, his balance will not increase at all. That's what I already told you in my first reply. From that moment on, both users will start earning fees on their position. User2 earns fees faster, but as we already said multiple times. Speed does not matter and is not an issue

His balance will be much bigger at some point later, compared to the other user. How does speed not matter if the exposure is easily increased? I truly do not understand why this would invalidate this attack. It just mitigates it up to a certain amount. When a fix is not fully correct, as in this case, it should be valid.

At another point in time fees for user1 will also equal principal. Both users balance is now exactly the same. No difference for the protocol between user1 and user2 (you could even argue that user2 is at a slight disadvantage since they have now deposited more liquidity fir a smaller balance hence are less capital efficient as user1).

This is false as the liquidity of user 2 is still accruing fees.

Nothing is not circumvented, nothing is exploited, nowhere does it lead to loss of funds for any user or the protocol, the final exposure towards the protocol is equal.

The final exposure is the same in a big enough time frame, in the short term, one of the users has a much bigger exposure.

And also it is not at no cost. User2 actually has to add the liquidity.

The 2 users in the example I gave had the same liquidity, so this argument does not hold.

Both the balance of user1 and user2 can grow from their properly deposited 100k to maximally 200k. That user2 improperly deposited another 100k in the lp position does not change that.

Again, this does not tell the whole story as in the meantime the exposure would have become much bigger than expected.

That 100k IS a real cost with no direct or long term benefit. Only affects the speed at which fees accumulate. (Which is not an exploit, user 2 could also have deposited in a pool with twice the trading volume and then fees would also accrue twice as fast).

This is false, the example I gave assumed both had the same liquidity, so no cost. They get the fees anyway from their liquidity so the benefit is there. This is literally cost free, assuming both have liquidity, to game the protocol.

0x73696d616f commented 6 months ago

For consistency sake, this should be judged the same as #18. The cap works for #18 because there is no better fix. However, in this attack vector, a complete fix is possible. It's not a matter of design, increasing the exposure past the limit maliciously in a much shorter time frame is an exploit and should be fully mitigated.

Thomas-Smets commented 6 months ago

His balance will be much bigger at some point later, compared to the other user. How does speed not matter if the exposure is easily increased? I truly do not understand why this would invalidate this attack. It just mitigates it up to a certain amount. When a fix is not fully correct, as in this case, it should be valid.

  1. It can't be much bigger, as it can never be more than 2x bigger as the principal, which is always smaller than max exposure (and in reality much much smaller since there are already other users in the protocol)
  2. Speed does not matter, I already gave you the example that if user deposited properly in a different pool with double the trading volume his fees would also accrue double. That is completely analogous.
  3. What is your attack? How does anyone lose funds because of this?
  4. It is indeed capped up to a certain amount. Which is a valid fix.

This is false as the liquidity of user 2 is still accruing fees.

No when the fee amount equals pronciple amount fees accrue but are not counted towards the balance. This is not something I should still have to explain since this is the whole point of the discussion!

Again, this does not tell the whole story as in the meantime the exposure would have become much bigger than expected.

What I wrote here is the full story. There is nothing I left out, the exposure of both positions cannot become bigger as 200k. Which is as expected, please show with a POC how the exposure can become bigger than 200k.

This is false, the example I gave assumed both had the same liquidity, so no cost. They get the fees anyway from their liquidity so the benefit is there. This is literally cost free, assuming both have liquidity, to game the protocol.

Let's say both have the same liquidity (lets say 200k). And both can maximally deposit 100k correctly.

User1 deposits 100k. -> max balance the position represents for the protocol can be 200k -> can use his other 100k somewhere else to earn yield

User2 deposits 100k ands 100k directly to lp. -> max balance the position represents for the protocol can be 200k -> His full liquidity is locked (he has clearly a big opportunity cost since he has to actually lock an additional 100k!)

Calling this fir free while you lock up double the liquidity is intellectually dishonest.

Since I am getting very tired of this yes - no game. Please use numerical examples, and write a poc how user2 with a proper deposit of 100k can get more than 200k in exposure.

Thomas-Smets commented 6 months ago

For consistency sake, this should be judged the same as #18. The cap works for #18 because there is no better fix. However, in this attack vector, a complete fix is possible. It's not a matter of design, increasing the exposure past the limit maliciously in a much shorter time frame is an exploit and should be fully mitigated.

No, you are now inventing rules. It is different as #18 as in #18 the cap could be circumvented completely. Here there is already a working cap in place.

0x73696d616f commented 6 months ago

It can't be much bigger, as it can never be more than 2x bigger as the principal, which is always smaller than max exposure (and in reality much much smaller since there are already other users in the protocol)

This is not an argument, much is not even objective.

Speed does not matter, I already gave you the example that if user deposited properly in a different pool with double the trading volume his fees would also accrue double. That is completely analogous.

You can't compare 2 different pools. Assuming the same pool, some user that adds liquidity after depositing, will have more balance and possible more over the exposure cap than a user that does not exploit.

What is your attack? How does anyone lose funds because of this? The attack is increasing exposure past the maximum.

It is indeed capped up to a certain amount. Which is a valid fix. It's not complete for this issue. Not a valid argument.

His full liquidity is locked (he has clearly a big opportunity cost since he has to actually lock an additional 100k!)

What do you mean he has an opportunity cost if he deposited the extra 100k in the liquidity pool and is earning fees. It's what he wanted to do anyway, but he also increases the balance on SlipstreamAM, win win.

No, you are now inventing rules. It is different as https://github.com/sherlock-audit/2024-04-arcadia-pricing-module-judging/issues/18 as in https://github.com/sherlock-audit/2024-04-arcadia-pricing-module-judging/issues/18 the cap could be circumvented completely. Here there is already a working cap in place.

Yes but until the cap it's free game.

0x73696d616f commented 6 months ago

We should move on with the semantics, the issue presents a valid way to increase the USD balance, possibly past exposure, in a way that was not supposed to happen. The attack has no cost as the user intended to use its liquidity anyway. Some parts of the code show the use of the cached liquidity, so it is a concern for you. In WrappedAerodromeAM, using cached liquidity may lead to lost rewards on some edge cases, but you still use it, so it certainly is a concern for you.

Thomas-Smets commented 6 months ago

You can't compare 2 different pools. Assuming the same pool, some user that adds liquidity after depositing, will have more balance and possible more over the exposure cap than a user that does not exploit.

If course you can, the maxExposure is in usd value, the underlying pool does not matter.

And the balance does not increase by increasing liquidity, stop saying this, or come with a poc that shows how.

What do you mean he has an opportunity cost if he deposited the extra 100k in the liquidity pool and is earning fees. It's what he wanted to do anyway, but he also increases the balance on SlipstreamAM, win win.

Again his balance does not increase. You admitted in your first reply in this conversation.

What you call an "exploit" is a very capital inefficient way to earn fees faster. His balance did not increase. The final max exposure (how much you can lend against it is the same). Users have no inscentives to do this at all. And if they which to do they can. There is no additional risk to the protocol.

Since you still do not provide numerical examples or a POC, I no longer believe this is a discussion in good faith.

Thomas-Smets commented 6 months ago

Also your solution does not prevent an actual attack as described in #18 only an actual cap on fees does. So the sotion is not valid.

0x73696d616f commented 6 months ago

If course you can, the maxExposure is in usd value, the underlying pool does not matter.

Yes but not in the intended pool.

And the balance does not increase by increasing liquidity, stop saying this, or come with a poc that shows how.

It increases up to the cap, over time, faster, than users that do not add liquidity in the same pool.

What you call an "exploit" is a very capital inefficient way to earn fees faster. His balance did not increase. The final max exposure (how much you can lend against it is the same). Users have no inscentives to do this at all. And if they which to do they can. There is no additional risk to the protocol.

Why is it inefficient if the user intended to only add liquidity to the pool anyway? it still earns fees. The balance increases with the fees, up to 2x the exposure cap or principal. The risk is increasing exposure more than users should. Again, why did you use the cached liquidity here?

Also your solution does not prevent an actual attack as described in https://github.com/sherlock-audit/2024-04-arcadia-pricing-module-judging/issues/18 only an actual cap on fees does. So the sotion is not valid.

My solution should be placed on top of the cap. The cap stops the USD balance from growing unbounded. My fix stops people from increasing the USD balance maliciously, if they have reached the cap (the actual cap up until you can deposit).

0x73696d616f commented 6 months ago

I do not know why the protocol is being so insistent on this, there is a clear discrepancy in the code between contracts. Using cached liquidity is something the protocol has implemented in some places but not all. Here it can be used to increase USD balance and possible exposure. It's something that was overlooked but for some reason they keep on denying.

Thomas-Smets commented 6 months ago

If a user has a value of X properly deposited. The total value of the position due to fees can maximally be 2X.

The only thing a user can do is manipulate the speed how fast his position increases from X to 2X.

What you describe as an attack is a very slow and inefficient way to increase this speed faster.

You could actually still use the way as explained in #18 to go from X to 2X faster than what you are talking about here. Even if we apply everything you say, you can still use #18 to go to 2X fast.

We do not care how fast you can go to 2x, we care that there is a maximum that cannot be exceeded

Thomas-Smets commented 6 months ago

I do not know why the protocol is being so insistent on this, there is a clear discrepancy in the code between contracts. Using cached liquidity is something the protocol has implemented in some places but not all. Here it can be used to increase USD balance and possible exposure. It's something that was overlooked but for some reason they keep on denying.

It was not overlooked, using cashed liquidity for the fees does not prevent the manipulation as mentioned in #18. Hence there is no point to use it and to increase gas costs and complexity.