KupiaSec - Adding liquidity can be `DoS`ed due to calculation mismatches

[sherlock-admin3]

KupiaSec

high

Adding liquidity can be DoSed due to calculation mismatches

Summary

When users add liquidity, they send tokens to the ArrakisPublicVaultRouter contract. The ValantisHOTModulePublic contract then takes the required tokens from the ArrakisPublicVaultRouter contract. However, due to a calculation mismatch, the required amount is often greater than the user-sent amount, causing the transaction to be reverted.

Vulnerability Detail

Let's consider following scenario:

1. The current state:
   • pool: reserve0 = 1e18 + 1, reserve1 = 1e18 + 1
   • vault: totalSupply = 1e18 + 1
2. Bob calls the ArrakisPublicVaultRouter.addLiquidity() function with the following parameters:
   • amount0Max = 1e18, amount1Max = 1e18
3. At L139, the _getMintAmounts() function returns:
   • (sharesReceived, amount0, amount1) = (1e18 - 1, 1e18 - 1, 1e18 - 1)
4. The router contract takes token0 and token1 from Bob in amounts of 1e18 - 1 each and calls the _addLiquidity() function with above parameters.
5. In the _addLiquidity() function, ArrakisMetaVaultPublic.mint(1e18 - 1, Bob) is invoked at L898.
6. In the ArrakisMetaVaultPublic.mint() function:
   • at L58, the proportion is recalculated as 1e18 - 1
   • _deposit(1e18 - 1) is called at L71
   • in the _deposit() function, ValantisHOTModulePublic.deposit(router, 1e18 - 1) is invoked at L150
7. In the ValantisHOTModulePublic.deposit() function:
   • amount0 = 1e18, amount1 = 1e18(at L71, L73)
   • at L79, L80, it takes token0 and token1 from the router in amounts of 1e18 each

Finally, the process fails because there is only 1e18 - 1 in the router, as mentioned in step 4.

This problem occurs because the calculations in the ArrakisPublicVaultRouter._getMintAmounts() function rely on rounding down. In contrast, the proportion calculation in the ArrakisMetaVaultPublic.mint() function and the amount calculations in the ValantisHOTModulePublic.deposit() function are based on rounding up.

Impact

Adding liquidity can be DoSed due to the calculation mismatches.

Code Snippet

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/ArrakisPublicVaultRouter.sol#L122-L191

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/ArrakisPublicVaultRouter.sol#L869-L901

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/ArrakisPublicVaultRouter.sol#L1194-L1231

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/ArrakisMetaVaultPublic.sol#L51-L74

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/ArrakisMetaVaultPublic.sol#L137-L154

https://github.com/sherlock-audit/2024-03-arrakis/blob/main/arrakis-modular/src/modules/ValantisHOTModulePublic.sol#L35-L96

Tool used

Manual Review

Recommendation

The ArrakisPublicVaultRouter._getMintAmounts() function should be updated to return the accurate required amounts.

[KupiaSecAdmin]

Escalate

The severity of this issue should be high, as the probability of the calculation mismatch occurring is quite high.

[sherlock-admin3]

Escalate

The severity of this issue should be high, as the probability of the calculation mismatch occurring is quite high.

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.

[WangSecurity]

In Sherlock, we don't judge issues based on the probabilities. As I understand there's no material loss, but the issue has serious non-material losses, cause as I understand, it's possible that there will always be users who cannot deposit due to this issue. Hence, I agree that high severity is appropriate here, planning to accept the escalation and upgrade severity to high.

[Gevarist]

Valid finding, for us it's a low level finding.

[CergyK]

In Sherlock, we don't judge issues based on the probabilities. As I understand there's no material loss, but the issue has serious non-material losses, cause as I understand, it's possible that there will always be users who cannot deposit due to this issue. Hence, I agree that high severity is appropriate here, planning to accept the escalation and upgrade severity to high.

This should remain medium, as there is no loss of funds.

In the hypothetical case some deposit absolutely has to be carried on, the simple work-around of sending some DUST of tokens to the contract would unlock the situation.

It seems that Sherlock has a new rule to identify high issues which is cited here: 2. Inflicts serious non-material losses (doesn't include contract simply not working). Which seems inconsistent with how to identify a medium issue: 2. Breaks core contract functionality, rendering the contract useless or leading to loss of funds. As the rule to identify the medium issue uses a higher impact?

[KupiaSecAdmin]

@CergyK

In the hypothetical case some deposit absolutely has to be carried on, the simple work-around of sending some DUST of tokens to the contract would unlock the situation.

Sending DUST of tokens cann't resolve the problem.

In the _addLiquidity() function of the router contract, the contract approves tokens to the module. Subsequently, in the deposit() function of the module, the module brings those tokens from the router contract. For the transaction to succeed, the router contract must approve enough tokens to the module. Therefore, only sending tokens cannot resolve the problem.

IERC20(token0_).safeIncreaseAllowance(module, amount0_);

token0.safeTransferFrom(depositor_, address(this), amount0);

Impossibility of deposit should be considered as a HIGH severity issue.

[CergyK]

@CergyK

In the hypothetical case some deposit absolutely has to be carried on, the simple work-around of sending some DUST of tokens to the contract would unlock the situation.

Sending DUST of tokens cann't resolve the problem.

In the _addLiquidity() function of the router contract, the contract approves tokens to the module. Subsequently, in the deposit() function of the module, the module brings those tokens from the router contract. For the transaction to succeed, the router contract must approve enough tokens to the module. Therefore, only sending tokens cannot resolve the problem.

IERC20(token0_).safeIncreaseAllowance(module, amount0_);

token0.safeTransferFrom(depositor_, address(this), amount0);

Impossibility of deposit should be considered as a HIGH severity issue.

Ok I see. Indeed depositing would be impossible in that case. Still this is a medium severity issue since it is a DOS of a core functionality, but no loss demonstrated

[KupiaSecAdmin]

IV. How to identify a high issue:

2. Inflicts serious non-material losses (doesn't include contract simply not working).

Based on the judging rule, I think the issue deserves high severity, because it inflicts serious non-material losses, which is not contract simply not working.

[0xjuaan]

@KupiaSecAdmin that rule actually says that there must be a non material loss other than the contract simply not working.

Since this bug simply causes a function to not work, I agree with @CergyK that this should be medium.

[WangSecurity]

@0xjuaan could you please elaborate? The rule says non-material losses, excluding the contract simply not working. Here, the contract is working, but the deposits are DOSed?

Also, another question, are the contracts upgradable and in reality could this issue be solved by updating the contract?

[0xjuaan]

It seems similar to this issue which is medium severity.

From what I understand, the rules say that the non-material loss must not be that a contract/function simply is not functional. However in this bug, that is the impact.

Note I did submit this so am not incentivised to disagree with the escalation, but it based on the rules I don't see any way this can be high severity.

[WangSecurity]

@0xjuaan could you please elaborate on what do you think includes "serious non-material losses"? So I can better understand how I can explain my decision better.

[WangSecurity]

The decision remains the same. I believe in that case it's not just a function simply doesn't work, but deposits into protocol are DOSed, hence, it's serious non-material losses. Planning to accept the escalation and upgrade severity to high.

[0xjuaan]

Honestly I still think @CergyK's stance is correct since deposits are technically not DoS'd, they can occur via the vault directly.

I do benefit from it being converted to H though so I don't mind either way.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/ArrakisFinance/arrakis-modular/pull/88

[CergyK]

@0xjuaan could you please elaborate on what do you think includes "serious non-material losses"? So I can better understand how I can explain my decision better.

One possible interpretation is that the line in the Sherlock rules:

2. Inflicts serious non-material losses (doesn't include contract simply not working).

was added to enable high severity issues in protocols which are not handling financial assets (SocialFi, data-availability). Otherwise it is too vague, and can be applicable to any valid issue (low, medium, high).

[WangSecurity]

Honestly I still think @CergyK's stance is correct since deposits are technically not DoS'd, they can occur via the vault directly.

Hm, I may have misunderstood that point. As I understand @CergyK talks about sending dust amounts to the contract, to mitigate the DoS. But this was answered here, which @CergyK agreed with. If I’m missing something please correct.

was added to enable high severity issues in protocols which are not handling financial assets (SocialFi, data-availability)

Fair point, but I still believe DOS of depositing into the protocol is indeed serious non-material loss.

But correct me on the first if I’m wrong.

[WangSecurity]

The decision remains the same. Planning to accept the escalation and upgrade severity to high.

[WangSecurity]

Result: High Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• KupiaSecAdmin: accepted

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.