decred / dcrdex

The Decred Decentralized Exchange (DEX), powered by atomic-swaps.
Other
186 stars 95 forks source link

eth/harness: Add test for contract overflow in init. #1720

Closed JoeGruffins closed 2 years ago

JoeGruffins commented 2 years ago

I was looking at this: https://gist.github.com/chappjc/350aafb9031f7a66986967bf8ab67d38

And there's a comment at the top:

 * Arithmetic operations in Solidity wrap on overflow. This can easily result
 * in bugs, because programmers usually assume that an overflow raises an
 * error, which is the standard behavior in high level programming languages.
 * `SafeMath` restores this intuition by reverting the transaction when an
 * operation overflows.

So I went to check our contract to see if overflow could affect us somehow. It looks like one could init several swaps together in such a way that they overflow and the sum is equal to the input value, but the values we store in the map could be anything. Later, this person could refund any amount at all.

But, this should not be the case since v0.8.0 https://docs.soliditylang.org/en/v0.8.13/080-breaking-changes.html

The transaction should be reverted if there is overflow for us currently.

We should at least make a harness test to ensure we are safe. Maybe even add checks for overflow to the contract(s)? Or at least some comments that version must be over v0.8.0 Adding more checks would up the transaction costs.

The value here is a uint256.

https://github.com/decred/dcrdex/blob/78dc76557a24fe798732c03f7ad9cceda8a0f6c6/dex/networks/eth/contracts/ETHSwapV0.sol#L92-L117

https://github.com/decred/dcrdex/blob/78dc76557a24fe798732c03f7ad9cceda8a0f6c6/dex/networks/eth/contracts/ETHSwapV0.sol#L171-L187

JoeGruffins commented 2 years ago

Got a thumbs up so will make the test, which passes on v0.8.15 we assume, and add some comments to the contracts near the pragma version.