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);
  • Is everyone from your _users[] list allowed to take all the ETH from the contract? Because If I am looking it right, they can clean the entire balance.
    – Sky
    Commented Feb 23, 2022 at 10:32
  • Yes It's a bot that I want to use with many wallets so It's okay
    – M M
    Commented Feb 24, 2022 at 14:50

2 Answers 2


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 ).

  • Thanks. I've added your code to the function. Actually there's no need to track users deposit and withdrawals because it's going to be used by only one person from different wallets.
    – M M
    Commented Jul 13, 2021 at 18:36


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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.