Savvy LP providers can steal value from other providers by sandwitching swap transactions

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xc119c58d4d3d2a6025d253b920d4a945b12a90fd3e30cb7591ae2b485893ecd9 Severity: high

Description: Description\ One factor that influences traders to use a specific platform for swapping tokens is the depth of liquidity available. The more the avalable liquidity, the better the price, less price impact and higher chances of executing high-volume trades. Many protocols incentive this behaviour while protecting it at the same time.

Thorn protocol looks to be vulnerable to this exploit, allowing providers to deposit and withdraw at will. The effect is that savvy providers can reap benefits where they have risked none.

This vulnerability is also present with the StableSwapTwoPool::donate_admin_fees method where the admin is expected to donate fee to LP providers Attack Scenario\ Consider the following scenario:

1. Customer calls StableSwapRouter::exactInputStableSwap looking to swap tokens.
2. Savvy trader A notices this transaction in the mempool and sends a sandwitch attack composed of:-

• a front-run call that deposits coins

• a back-run call that withdraws all coins plus rewards gained

  The effect is that the savvy trader has risked none while sharing benefits with other LP providers.

Attachments

1. Proof of Concept (PoC) File Consider this is POC that shows such an attack:

    it("should unfairly share rewards", async () => {

        await (await stROSE.mint(accounts[1].address, ADD_AMOUNT.mul(10))).wait();
        await (await stROSE.connect(accounts[1]).approve(stableSwap2Pool_ROSE_stROSE.address, ADD_AMOUNT)).wait();
        await (await stableSwap2Pool_ROSE_stROSE.connect(accounts[1]).add_liquidity([ADD_AMOUNT, ADD_AMOUNT],0,{ value: ADD_AMOUNT })).wait();

        let tokensMinted: BigNumber = await stableSwapLP.balanceOf(accounts[1].address);

        let account_one_before_stROSE: BigNumber = await stROSE.balanceOf(accounts[1].address);
        let account_one_before_ROSE: BigNumber = await ethers.provider.getBalance(accounts[1].address);

        await stableSwapRouter.exactInputStableSwap([ROSE,stROSE.address], [2],0, 0,accounts[2].address,{value: ethers.utils.parseEther("1.0")});

        await stableSwap2Pool_ROSE_stROSE.connect(accounts[1]).remove_liquidity(tokensMinted,[0, 0]);

        let account_one_after_stROSE: BigNumber = await stROSE.balanceOf(accounts[1].address);
        let account_one_after_ROSE: BigNumber = await ethers.provider.getBalance(accounts[1].address);
        //expect(account_one_after_stROSE).to.be.greaterThan(account_one_before_stROSE);
        //expect(account_one_after_ROSE).to.be.greaterThan(account_one_before_ROSE);

    });

2. Revised Code File (Optional) Consider implementing a minimum deposit period to prevent LP providers from withdrawing immediately after depositing.

[omega-audits]

This is not a "sandwhich attack", this is someone providing "just in time" liquidity, which is behavior that is useful and to be rewarded, and the "attacker" is rightly getting rewarded. for that.

More precisely, the "attacker" has exactly the same risks and costs (i.e. deposit fees, impermanent loss) as all other LP providers. The "attack" will probably not be profitable anyway (as usually the cost of IL alone will be higher than the profit from the fees) The extra liquidity is good for the trader, who will have less slippage.

[Ngugs]

How about profiting from donate_admin_fees ? Attacker's liquidity has added no value to the protocol ? The reason I combined this 2 vulns into 1 report is because the root cause is the same ...

[omega-audits]

Ok, I missed that part :) So the issue here would be not about normal users losing value, but for an attacker to siphon of part of the admin fees, if these are donated to the pool. Your proposed solution does not really work here, though, both because (a) we definitely want to allow for (very) short term liquidity and (2) it would only mitage, not solve, this problem. I am not sure if an in-contract solution exists, and perhaps it is not even necessary: given that this is an admin function, it is probably called by a knowledgeable users, that can use private mempools if frontrunning becomes a problem.

[Ngugs]

I want to focus on the admin function vulnerability. You have stated that , "given that this is an admin function, it is probably called by a knowledgeable users, that can use private mempools if front-running becomes a problem." This report has stated that the donate_admin_fees is prone to front-running, therefore it is not in order to state that the admin can use private mempools IF front-running becomes a problem. It is now a known vulnerability.

"it is probably called by a knowledgeable users, that can use private mempools' - I think this statement also not in order. Your statement is an after-thought using information from the report. This contest was NOT aware of the vulnerability reported here. The list of known issues section on the contest page does not mention this vulnerability and neither do the docs nor the function's NatSpec. Any researcher would therefore fairly believe that admin would have no cause to use private mempools. The strongest reason why someone would use a private mempool would be front-running- AFTER knowing that they're prone.

I don't understand why you say that the attacker would siphon part of admin fee. Your comment suggests that there's a partition between swapping fee and tokens that have been donated by admin. I don't think the protocol works that way. The reality is that attacker would benefit from an increase in the pool fees just like any LP provider.

My suggested fix might not be a permanent solution, otherwise that would require developing more complex fee-sharing mechanisms. A suggested fix in this contest is desirable if the reporter can provide one.

[omega-audits]

My main point here was that there is no clear on-chain fix to the problem of front-running a call to donate_admin_fees, while there are off-chain solutions, and so the problem cannot be seen as an issue with the code.

[Ngugs]

Ok. I believe that this report has served its purpose, i.e. surface a vulnerability that technically allows non-LP providers to earn from admin donations, basically stealing a share from other LP providers. As to the perfect solution, there are several possible approaches. This is a task that the protocol should consider to protect worthy LP providers.

[0xSwahili]

I think I have figured out the perfect solution:

1. Admin scans the blockchain and picks the latest block number X.
2. An announcement is made that LP providers till block X will share the admin fee.
3. Admin builds a Merkle-trie of all recipients and amounts and commits the data to the blockchain.
4. Beneficiaries withdraw tokens at their own leisure.

This approach ensures that only intended beneficiaries receive the rewards.