search

0x00000002 / staking-app

MIT License
0 stars 0 forks source link

Check overflow. #16

Open ryu9827 opened 6 years ago

ryu9827 commented 6 years ago

https://github.com/tikonoff/staking-app/blob/8306bb8deebe0f554e99bf4e525ea4a3f5672397/contracts/StandardToken.sol#L19 It is better to check overflow all the time. Don't make assumption that your variables are safe.

0x00000002 commented 6 years ago

I'm not sure that's a case. They don't really add something valuable to storage. They just check that the balance of the sender is greater than amount. I don't think someone really have 2*256 ETH on their account even counted in Weis... (there are no so many ETH in the whole world).. End to imagine that such number could be SENT to buy LEVs token.. Not to mention that there are 1.000.000.000 LEVS only, with price 1LEV = 100000 weis? Definitely far away from 2256.

ryu9827 commented 6 years ago

balances[msg.sender] is the balance of msg.sender's LEV token, not the balance of his ETH. If the price is 1 LEV = 100,000weis, and you send 1 ETH to buy LEV token, you get 10,000,000,000,000 tokens back. Seem closer to 2**256 ? Like what I said: Always check overflow. Don't make assumption that your variables are safe.

0x00000002 commented 6 years ago

You can compare it by yourself:
10,000,000,000,000 vs. 115,792,089,237,316,195,423,570,985,008,687,907,853,269,984,665,640,564,039,457,584,007,913,129,639,936

It is not always possible to check something before saying that, but to check everything is an essence of our job, especially before writing any recommendations to the client.

Also, I believe that we should use the words like "should", "have to", "must" or "always" only when addressing ourselves and avoid to use them towards anyone else. I'm not sure if any client in the world wants to get such orders from us.

On the contrary, we can use sentences like "It is better to use...", "It is considered safe ... ", "The common practice is ..." and so on.

I believe. :)

ryu9827 commented 6 years ago

I still insist to recommend him to check overflow even it seems far from 2**256. But I can improve the words in the sentences. Thank you for the advise : )

Janther commented 6 years ago

@ryu9827 @tikonoff Even if this is an issue to be considered, I would downgrade it from Critical to Moderate.

0x00000002 commented 6 years ago

Actually I still believe it is Minor because of:

Klaus, could you please explain why do you think it is Moderate?

Janther commented 6 years ago

Minor - A defect that does not have a material impact on the contract execution and is likely to be subjective.

yeah, according to this, you are right, my mistake.