cergyk - ArrakisStandardManager::rebalance Malicious executor can bypass slippage check and steal funds from a public vault

[sherlock-admin2]

cergyk

medium

ArrakisStandardManager::rebalance Malicious executor can bypass slippage check and steal funds from a public vault

Summary

A malicious executor can bypass the reserves checks at the end of ArrakisStandardManager::rebalance, by using a malicious contract as _router during a ValantisModule::swap call and using it to deposit through ArrakisPublicVaultRouter::addLiquidity. This would increase reserves temporarily in order to pass the checks, but the added funds can be withdrawn by the executor after rebalance is done.

Vulnerability Detail

Checks preventing funds extraction by executor

There are two levels of checks to bypass in order to accomplish the full attack:

1/ Slippage checks in ValantisModule::swap limit slippage for a given swap operated by the router to a reasonable value (let's assume 1%):

• check expectedMinReturn: ValantisHOTModule.sol#L335-L342

• and then: ValantisHOTModule.sol#L384-L397

2/ Reserves held by the pool are checked at the end of rebalance: ArrakisStandardManager.sol#L391-L414

Bypassing the checks

Let's see how to bypass these checks:

1/ Since we are allowed an unlimited number of arbitrary calls to the module during rebalance, we can use many swaps each incurring an allowed 1% slippage (which is going to the attacker). This means that for 50 swaps approx 40% of the TVL of the vault is out: ArrakisStandardManager.sol#L364-L379

Alternatively one could manipulate the pool to incur slippage on the depositLiquidity call in swap, since the slippage controls are controlled by the executor: ValantisHOTModule.sol#L406-L411

2/ Now that an arbitrary amount of funds is extracted from the pool, we need to use one last call on the module, in order to addLiquidity via the ArrakisPublicVaultRouter and inflate reserves.

To do so, we execute a ValantisModule::swap, with amountIn being zero, and use an attacker provided contract as router to call on ArrakisPublicVaultRouter::addLiquidity.

The deposited funds will increase the reserves back to their initial values, except the executor can withdraw these funds after rebalance is done.

Impact

Theft of arbitrary amount of funds from public vaults by a malicious executor

Scenario

1. The malicious executor calls ArrakisStandardManager::rebalance with a crafted payloads_ that includes:

   • 50 calls to module::swap with an attacker controlled routerA, each keeping 1% of amountIn and sending tokenOut
   • 1 call to module::swap with attacker controlled routerB, which calls on ArrakisPublicVaultRouter::addLiquidity and deposits the missing liquidity back, but claimable by the attacker

2. The malicious executor calls ArrakisPublicVaultRouter::removeLiquidity, and withdraws 40% of the available reserves in the router

Code Snippet

• ArrakisStandardManager.sol#L376

Tool used

Manual Review

Recommendation

1/ Instead of checking available reserves at the end of ArrakisStandardManager::rebalance, please consider checking the share price reserves/totalSupply

a private vault does not implement ERC20 and does not keep accounting of shares, so a virtual arbitrary share value such as 1e18 can be used. This is safe because it is considered that only the private vault owner can deposit into a private vault, so this means that the executor should not be able to do step 2. above.

2/ Additionally, one can add deviation checks on slippage controls provided by the executor here: ValantisHOTModule.sol#L406-L411

3/ Finally, add a whitelist of approved routers for handling swaps in modules.

[IWildSniperI]

this issue should be grouped with other malicious _router calls for the following reason

what happens if we actually verify _router calls? will any of those issues still be valid after? no?, then this is what is called "same root cause"

[0xjuaan]

Escalate

This issue and #55 do not show sufficient proof to be considered dupes of #44

Summary of vulnerability for reference

Rebalancing has 3 steps: withdraw all funds -> swap -> deposit all funds back

However, instead of swapping in step 2, a malicious executor can use an extremely cheap share price to mint a large amount of shares, which they can withdraw for nearly 100% of the vault's liquidity after step 3 of rebalance() occurs.

Key constraints

There are a few key constraints to this attack:

1. In the middle of the swap() function (after the liquidity has been withdrawn), the attacker must increase the reserves of the pool to make it non-zero. Otherwise, depositing liquidity to mint cheap shares will revert due to SovereignPool__depositLiquidity_zeroTotalDepositAmount

2. In order for the attacker to be able to do the above, at least one of the tokens must be a rebase token, since otherwise the reserves are calculated via storage variables and cannot be updated by sending dust amounts of tokens.

3. expectedMinReturn_ tokens have to be sent into the contract between step1 and 3. And due to _checkMinReturn, amountIn for the swap has to be non-zero, with the lowest possible value being 1. This means the lowest possible value for expectedMinReturn_ is 1 wei, so in between step1,3- the attacker has to send 1 wei worth of tokens into the module to ensure that the check passes.

Without any of these, the attack would fail.

You can find all these constraints detailed in #44

Sherlock rules

Now, the sherlock rules state the following:

PoC is required for all issues falling into any of the following groups:

• non-obvious ones with complex vulnerabilities/attack paths
• issues for which there are non-trivial limitations/constraints on inputs, to show that the attack is possible despite those

Clearly, this vulnerability meets the two above criteria, since it is a complex attack with several requirements for success.

Explanation of how #76 and #55 do NOT show how the attack is possible despite the various constraints:

However, this submission and #55 are lacking the following key features:

1. They do not explain that the attacker must increase the reserves of the pool during the callback so that the minting of shares does not revert
2. They do not explain that at least one of the tokens must be rebase in order for this attack
3. They do not explain that during the callback, expectedMinReturn tokens must be sent into the module during the callback.
4. They do not include a PoC as required by the sherlock rules for a complex vulnerability with various non-trivial limitations and constraints.

This submission also states:

To do so, we execute a ValantisModule::swap, with amountIn being zero

However this is an incorrect attack path since passing amountIn=0 will revert within ValantisHOTModule.checkMinReturn due to division by zero here.

Conclusion

Based on the above evidence (lack of explaining key constraints, lack of PoC, incorrect attack paths), it is clear that #76 and #55 are insufficient reports based on sherlock's rules and cannot be duped with #44.

Note that #44 explains ALL the non-trivial limitations/constraints of the issue, and shows how the attack is possible despite those. It also provides a coded PoC that demonstrates 100% of the vault's funds being stolen. On the other hand, the attack paths provided by #76 and #55 only show a smaller portion of the funds being stolen.

[sherlock-admin3]

Escalate

This issue and #55 do not show sufficient proof to be considered dupes of #44

Summary of vulnerability for reference

Rebalancing has 3 steps: withdraw all funds -> swap -> deposit all funds back

However, instead of swapping in step 2, a malicious executor can use an extremely cheap share price to mint a large amount of shares, which they can withdraw for nearly 100% of the vault's liquidity after step 3 of rebalance() occurs.

Key constraints

There are a few key constraints to this attack:

1. In the middle of the swap() function (after the liquidity has been withdrawn), the attacker must increase the reserves of the pool to make it non-zero. Otherwise, depositing liquidity to mint cheap shares will revert due to SovereignPool__depositLiquidity_zeroTotalDepositAmount

2. In order for the attacker to be able to do the above, at least one of the tokens must be a rebase token, since otherwise the reserves are calculated via storage variables and cannot be updated by sending dust amounts of tokens.

3. expectedMinReturn_ tokens have to be sent into the contract between step1 and 3. And due to _checkMinReturn, amountIn for the swap has to be non-zero, with the lowest possible value being 1. This means the lowest possible value for expectedMinReturn_ is 1 wei, so in between step1,3- the attacker has to send 1 wei worth of tokens into the module to ensure that the check passes.

Without any of these, the attack would fail.

You can find all these constraints detailed in #44

Sherlock rules

Now, the sherlock rules state the following:

PoC is required for all issues falling into any of the following groups:

• non-obvious ones with complex vulnerabilities/attack paths
• issues for which there are non-trivial limitations/constraints on inputs, to show that the attack is possible despite those

Clearly, this vulnerability meets the two above criteria, since it is a complex attack with several requirements for success.

Explanation of how #76 and #55 do NOT show how the attack is possible despite the various constraints:

However, this submission and #55 are lacking the following key features:

1. They do not explain that the attacker must increase the reserves of the pool during the callback so that the minting of shares does not revert
2. They do not explain that at least one of the tokens must be rebase in order for this attack
3. They do not explain that during the callback, expectedMinReturn tokens must be sent into the module during the callback.
4. They do not include a PoC as required by the sherlock rules for a complex vulnerability with various non-trivial limitations and constraints.

This submission also states:

To do so, we execute a ValantisModule::swap, with amountIn being zero

However this is an incorrect attack path since passing amountIn=0 will revert within ValantisHOTModule.checkMinReturn due to division by zero here.

Conclusion

Based on the above evidence (lack of explaining key constraints, lack of PoC, incorrect attack paths), it is clear that #76 and #55 are insufficient reports based on sherlock's rules and cannot be duped with #44.

Note that #44 explains ALL the non-trivial limitations/constraints of the issue, and shows how the attack is possible despite those. It also provides a coded PoC that demonstrates 100% of the vault's funds being stolen. On the other hand, the attack paths provided by #76 and #55 only show a smaller portion of the funds being stolen.

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.

[CergyK]

@0xjuaan is right in that the attacks described in #76 and #44 do not follow the same path:

• in #44 the attacker mints directly when reserves are zero between withdraw and deposit during module::swap

• in #76 the attacker mints with accumulated funds which are drained by successive slippage in multiple router.swaps

Both successfully bypass the restriction on reserves at the end of the rebalance.

As noted by @0xjuaan #44 works only when one token is rebasing, whereas #76 works with all types of tokens. I believe these issues are correctly duplicated since they use a share inflation during ArrakisStandardManager::rebalance and can be both mitigated by a share price check as recommended in #76.

I believe now that #55 is invalid since step 3. would not be possible without triggering the individual swap slippage check in ValantisHOTModule.sol#L383-L397, which is also not mentionned in the analysis.

As for the lack of PoC, It is my understanding that it is not straght away criteria for invalidation, but lead judge can ask a Watson to provide a PoC if something is unclear in a complex issue.

[IWildSniperI]

From sponser prospective Will be any of them valid if router calls are verified? No? Then those are the root causes which are the same

Malicious router call through malicious payload

[0xjuaan]

To re-iterate, the attack path stated in #76 is incorrect for the following reasons:

1. It does not explain the requirement that at least one token has to be a rebase token
2. It does not explain that the attacker MUST send tokens to the sovereign pool during the rebalance, to ensure that minting shares will not revert.
3. This report states that amountIn can be set to 0 but this would actually cause a revert. The lowest it can go is 1, and that forces expectedMinReturn to be 1 at lowest.
4. It does not state the expectedMinReturn tokens have to be transferred by the attacker to the module during the attack to prevent reverting.

The attack path provided will simply not work, since the non-trivial constraints (each constraint is explained in #44 and my escalation comment) are not considered in the report.

@CergyK has mentioned

Both successfully bypass the restriction on reserves at the end of the rebalance.

But this is a trivial constraint of the attack, and the attack path in this report does not bypass the 4 non-trivial constraints of this attack.

Here, the sherlock docs state:

Also, Watsons must outline all constraints of the issue being triggered and specify in which situations these constraints may trigger the issue.

Since the watson clearly did not outline all constraints of the issue and how to trigger the issue, this submission (along with #55 for similar reasons) should not be duped with #44.

[Gevarist]

Each vault has a cooldown period during which rebalance cannot happen. If attacker need 40 differents transactions to perform the attack, that will provide plenty of time for vault's owner to change the executor.

[CergyK]

To re-iterate, the attack path stated in #76 is incorrect for the following reasons:

1. It does not explain the requirement that at least one token has to be a rebase token

2. It does not explain that the attacker MUST send tokens to the sovereign pool during the rebalance, to ensure that minting shares will not revert.

3. This report states that `amountIn` can be set to `0` but this would actually cause a revert. The lowest it can go is `1`, and that forces `expectedMinReturn` to be 1 at lowest.

4. It does not state the `expectedMinReturn` tokens have to be transferred by the attacker to the module during the attack to prevent reverting.

The attack path provided will simply not work, since the non-trivial constraints (each constraint is explained in #44 and my escalation comment) are not considered in the report.

@CergyK has mentioned

Both successfully bypass the restriction on reserves at the end of the rebalance.

But this is a trivial constraint of the attack, and the attack path in this report does not bypass the 4 non-trivial constraints of this attack.

Here, the sherlock docs state:

Also, Watsons must outline all constraints of the issue being triggered and specify in which situations these constraints may trigger the issue.

Since the watson clearly did not outline all constraints of the issue and how to trigger the issue, this submission (along with #55 for similar reasons) should not be duped with #44.

@0xjuaan is correct in that this report and #55 are not entirely correct with regards to the full attack flow.

• This report misses the fact that reserves would be zero at the time of adding liquidity, and that it would need a rebasing token and donating some token to the pool to work (the requirement of amountIn != 0 is trivially bypassed by sending 1 wei).

• 55 misses the fact that adding liquidity would revert if reserves are zero, and that slippage checks during the individual swaps would not be bypassed (_expectedMinReturn).

All of these reports however correctly identify the same root cause which leads to the bug: A cross-contract reentrancy is possible during rebalance which enables to deflate the price of public vault shares.

This report and #55 arguably identify the more robust recommendation to solve this problem which is to check that the price of public vault shares has not decreased too much after rebalance

Will trust @WangSecurity judgement on this one.

[CergyK]

Each vault has a cooldown period during which rebalance cannot happen. If attacker need 40 differents transactions to perform the attack, that will provide plenty of time for vault's owner to change the executor.

The attack described in this report uses only 1 tx to manipulate the share price

[ADK0010]

@WangSecurity Maybe the Duplication Rules of Sherlock for this contest help in this case . It states - In addition to this, there is a submission D which identifies the core issue but does not clearly describe the impact or an attack path. Then D is considered low.

[sherlock-admin2]

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

[WangSecurity]

Thank you for these insightful comments. I believe @0xjuaan quoted a very correct line from POC requirements:

Also, Watsons must outline all constraints of the issue being triggered and specify in which situations these constraints may trigger the issue.

To clarify for the future, POC requirements also allow for written POC (not only coded but coded is amazing) and issues falling into groups inside the POC Requirements section should have the POC (either written or coded) initially.

Planning to accept the escalation to invalidate this report and make #44 the main one. About #55, let's keep the discussion there and discuss only #76 here.

[CergyK]

Thank you for these insightful comments. I believe @0xjuaan quoted a very correct line from POC requirements:

Also, Watsons must outline all constraints of the issue being triggered and specify in which situations these constraints may trigger the issue.

To clarify for the future, POC requirements also allow for written POC (not only coded but coded is amazing) and issues falling into groups inside the POC Requirements section should have the POC (either written or coded) initially.

Planning to accept the escalation to invalidate this report and make #44 the main one. About #55, let's keep the discussion there and discuss only #76 here.

For the record this report has a not coded PoC (see scenario section).

Obviously I'm sided to not invalidate this report since it identifies correctly the flow of the vulnerability (even after admitting the correct points that @0xjuaan makes).

The attack flow is fairly complex so not mentionning 1 or 2 trivial checks should not change the outcome, especially since this report and #55 mention a better recommendation than the one in the report #44.

[WangSecurity]

These are fair points, but the rules say "Watsons must outline all constraints**. Hence, I have to agree with the escalation and deem this report invalid.

Planning to accept the escalation and invalidate this report.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xjuaan: accepted

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.