Smart contract : wrong code or race condition?

Question

We have developed a simple Smart Contract which offers this following public transaction method:

struct Bid {
           uint256 userCode;
           uint256 amount;
           }

Bid public winningBid;
Bid[] public bids;

function bidAmount(uint256 _userCode, uint256 _amount) public {

        assert(_userCode> 0);
        assert(_amount> 0);            
        assert(_amount > winningBid.amount + winningBid.amount * (5/100));

        winningBid.userCode= _userCode;
        winningBid.amount= _amount;

        var bidData=Bid(_userCode, _amount);
        bids.push(bidData);
    }

The bidAmount method checks if the bid is valid verifying if the amount is bigger than the amount of the current winning bid plus a mandatory amount step (winningBid.amount * (5/100)) ; if the check is verified, the winning bid becomes the current winning bid and a new bid is pushed in the bids list.

How is it possible that we've been able to store in ethereum a bidding list like this?

22/01/2018 11:51 13.500,00 MrX
22/01/2018 11:51 13.440,00 MrY
22/01/2018 11:49 12.800,00 MrZ

MrX bid violates the Smart Contract check:

assert(_amount > winningBid.amount + winningBid.amount * (5/100));

It looks like a "race condition" where MrX and MrY biddings were both done just checking the validity on MrZ bid. What we expected is to have one of the two biddings fails for assert violation.

To the best of my knowledge, contract executions are serialised within a block and are not performed in parallel.
Is it simply bad coding or a race condition?

Answer 1

Ethereum only uses integer arithmetic and will truncate mathematical operations.

The expression, will calculate 5/100 first which is 0.

winningBid.amount * (5/100) = winningBid.amount * 0 = 0

The solution is to force the order of the operations to first multiply

require(_amount > winningBid.amount + (winningBid.amount * 5) / 100);

Also it is suggested to use require instead of assert to validate input, assert is for totally unexpected circumstances. After byzantinum require will cause a revert() returning the unused gas to the sender, and assert will consume all the remaining gas.