Is this function prone to underflow hacking?

Question

So I want to write as simple as possible withdrawal function in a token contract which also doesn't use Safemath for one very simple reason: I admit I don't understand well enough data types in EVM so I'll rather stick to basic logical statements to prevent hacking, so how safe is this:

function withdrawTokens(uint _amount) external returns (uint _available) {


        _available = _amount < balances[msg.sender] ? _amount :     balances[msg.sender];
        msg.sender.transfer(_available);
        balances[msg.sender] -= _available;
        emit eventWithd(msg.sender, _available);


    }

My major concern is this line:

    balances[msg.sender] -= _available;

This will obviously underflow if the balance of the msg.sender is say 5 and they send 6 as argument (5-6 = -1 but in uint it's huge positive number...), so I am betting on the fact that at least one of these 2 lines will revert thus never reaching the dangerous line above:

 _available = _amount < balances[msg.sender] ? _amount balances[msg.sender];
        msg.sender.transfer(_available);

Any thoughts? Thanx a lot!

Answer 1

So I am betting on the fact that at least one of these 2 lines will revert

The only line in your code which may revert is msg.sender.transfer(_available).

This line will revert if address(this).balance < _available.

If msg.sender is a contract, then it may revert also when address(this).balance >= _available.

Other than that, the line balances[msg.sender] -= _available will obviously not underflow, because your code ensures beforehand that balances[msg.sender] >= _available (and even if it did, such scenario would not cause the execution to revert).

Answer 2

Your code looks fine to me.

If you take a look at the SafeMath library's subtraction function, you can see that it works in a similar manner:

    function sub(uint256 a, uint256 b, string memory errorMessage) internal pure returns (uint256) {
        require(b <= a, errorMessage);
        uint256 c = a - b;

        return c;
    }

What you're doing is capping the amount to be subtracted to the actual balance of an account, which is totally fine. Any value that is subtracted will be between 0 and balance[msg.sender].

Also IMO there is no real reason not to use SafeMath tbh. The datatypes in solidty are limited, and with SafeMath you're only handling uint (which internally are uint256) types, and the code, as you can see above, is not really anything complex.