Solidity simple contract for withdrawing balance

I wrote a solidity contract for withdrawing balance of cantract. May someone tell if its working. withdrawSafe should prevent reentrancy attack, but i don`t know if it is. Just check if everything is okay, and please suggest how to improve it. Could have done some stupid mistakes.

// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.19;

event Response(bool success, bytes data);


interface IVault {
    function deposit() external payable;

    function withdrawSafe(address payable holder) external;

    function withdrawUnsafe(address payable holder) external;
}

interface IAttacker {
    function depositToVault(address vault) external payable;

    function attack(address vault) external;
}

contract Vault is IVault {

    bool private _entered;

     modifier nonReentrant {
        require(!_entered, "re-entrant call");
        _entered = true;
        _;
        _entered = false;
    }

    mapping(address => uint256) public balance;

    function deposit() external payable {
        balance[msg.sender] += msg.value;
    }

    function withdrawSafe(address payable holder) external nonReentrant {
        (bool success, bytes memory data) = holder.call{value: balance[msg.sender], gas: 5000}
        (abi.encodeWithSignature("withdrawSafe(string,uint256)", "call WS", 123));
        emit Response(success, data);
    }

    function withdrawUnsafe(address payable holder) external {
        (bool success, bytes memory data) = holder.call{value: balance[msg.sender], gas: 5000}
        (abi.encodeWithSignature("withdrawSafe(string,uint256)", "call WS", 123));
        emit Response(success, data);
    }
}

Also if someone knows how to test contracts in Remix without any testnet or mainnet chains, or maybe reccomend another IDE, i`ll be very glad to hear any suggesions.

Solution

I've encountered a few issues which can be improved:

The nonReentrant modifier is correctly implemented, but it should also be used to protect the balance update logic inside the withdrawSafe function.
The balance should be updated before making the external call to prevent reentrancy. This ensures that even if the external call makes a recursive call, the balance has already been set to zero, preventing reentrancy.
The current implementation does not update the balance of the holder. This is much needed to prevent reentrancy.
The gas limit specified (5000) may not be enough for the call to succeed, depending on the implementation of the receiver. You may want to ensure it's sufficient for the recipient to handle the transaction.
It's not necessary for a simple transfer of Ether. You can directly transfer funds using holder.transfer or holder.call{value: amount}("").

With suggested improvements, you contract will look something like this:

// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.19;

event Response(bool success, bytes data);

interface IVault {
    function deposit() external payable;

    function withdrawSafe(address payable holder) external;

    function withdrawUnsafe(address payable holder) external;
}

interface IAttacker {
    function depositToVault(address vault) external payable;

    function attack(address vault) external;
}

contract Vault is IVault {
    bool private _entered;

    modifier nonReentrant {
        require(!_entered, "re-entrant call");
        _entered = true;
        _;
        _entered = false;
    }

    mapping(address => uint256) public balance;

    function deposit() external payable {
        balance[msg.sender] += msg.value;
    }

    function withdrawSafe(address payable holder) external nonReentrant {
        uint256 amount = balance[msg.sender];
        require(amount > 0, "Insufficient balance");

        // Update balance before making the external call
        balance[msg.sender] = 0;

        // Use call to transfer funds and handle response
        (bool success, bytes memory data) = holder.call{value: amount}("");
        require(success, "Transfer failed");

        emit Response(success, data);
    }

    function withdrawUnsafe(address payable holder) external {
        uint256 amount = balance[msg.sender];
        require(amount > 0, "Insufficient balance");

        // Update balance before making the external call
        balance[msg.sender] = 0;

        // Use call to transfer funds and handle response
        (bool success, bytes memory data) = holder.call{value: amount}("");
        require(success, "Transfer failed");

        emit Response(success, data);
    }
}

This should help ensure that the withdrawSafe function's secure against reentrancy attacks.