Solidity: Safe way to withdraw from contract

Question

I have a contract that accepts ETH and turns it into WETH and keeps it. I have written a function to withdraw this tokens from contract. Is this function safe?

    modifier onlyUsers {
        require(_users[msg.sender] || msg.sender == _owner, "Not Allowed");
        _;
     }

    function withdrawTokensFromContract(address _token, uint256 _amount) public onlyUsers returns (bool) {
        uint256 balance = getTokenBalance(_token);
        require(balance >= _amount, 'Amount exceeds Balance');
        IBEP20(_token).transfer(msg.sender, _amount);
    }

Answer 1

First thing to add at the top of method withdrawTokensFromContract is:

require(_token != address(0), "Token cannot be zero address.");

With the current logic I don't see validation for each user deposited balance and one user can withdraw all of the contract tokens. You cannot track simple token transfers to the contract, but you can build custom "deposit" method which will be keeping records how many tokens have been deposited by each user.

Also make sure the .transfer() is always at the bottom of the method ( after the state update, in order to skip the reentrancy issues ).

Answer 2

THIS CONTRACT IS DRAINABLE

ok, so i saw your reply to a prevoious answer, saying that this contract will be used by one user with different wallets. That is equivalent to being used by multiple users.

Here you can have two courses of action to keep malicious actors from stealing your WETH: Keep track of deposited amount by user and don't allow anyone to withdraw from other user's deposits

You can do this by keeping balances in a mapping

mapping( address => mapping( address => uint) public balances;

and access data like this:

balance = balances[msg.sender][_token];

or be very careful to restrict access to only authorized users. Makes sense to have this logic implemented in the decorator onlyUsers you already have.