switching on SafeMath for uint256 in MultiPartyEscrow.sol

[astroseger]

switch on SafeMath for uint256 in MultiPartyEscrow.sol

[tiero]

It's not enough. The main point of SafeMath is using helper functions instead of native Solidity arithmetic.

eg. https://github.com/singnet/token-contracts/blob/master/contracts/AgiCrowdsale.sol#L101

[astroseger]

It makes sense.
Now we return to the main question. Which operation should be check.

1. "+= value" in deposit function? But this value was already checked token.transferFrom
2. "-= value" in withdraw function? But it was already checked in require (require(balances[msg.sender] >= value))
3. "-= value" in openChannel? But it was checked in require (require(balances[msg.sender] >= value)).
4. "+=amount", "-=amount" in channelClaim? But it was already checked in require (require(amount <= channel.value)).
5. "+=amount", "-=amount" in channelAddFunds? But it was already checked in require (require(balances[msg.sender] >= amount)).

[astroseger]

So I think we should check only "+= value" in deposit function, because we don't check it by ourselves . All other checks seems to be direct doublechecks (because we check it in require in the same function).... Of course, we can still double-check everything...

[ksridharbabuus]

Though we have the require conditions in place I will suggest to use the SafeMath helper functions for any math operations. This will make us aligned with the coding standards for contracts.

[astroseger]

I've added SafaMath for all operations in MultiPartyEscrow, even though I think it is not completely right to force innocent people to pay for our paranoia :) .

But difference in gas is not so catastrophic:
Difference in Gas prices (old/new):
channelClaim 65520 / 66116 (+596 gas)
channelClaimTimeout 29272 / 29485 (+213 gas)
channelExtendAndAddFunds 39246 / 39538 (+292 gas)
deposit 49146 / 49295 (+149 gas)
openChannel 139420/139657 (+237 gas)

And this modification should not be consider as an excuse to not write more tests: #46

[tiero]

Here's different. Use the add method also here to make it consistent

[astroseger]

No, It doesn't make sense here.

1. "1" doesn't come from outside word, we shoudn't check it.
2. overflow wouldn't be an issue here (even if we could overflow with +1, uint256), because logic of the channel will work, we only need nonce to be different, even bool would work here with some constrains in sender/recipient logic.

[astroseger]

I want to say it again, but I'm feeling rather strange arguing that we shouldn't make extra checks, because I usually teach people to always make extra checks. But let's not forget that here we pay real money for each operation. And adding extra cost only to be "consistent" is really really strange.
I want to underline again, MPE is very small contract, which will be used the most in SingularityNet, so our consumers will pay most of their gas here.

We need to be consistent only in the situation where we might forget to make checks in important places. But here it is not the case.
We are already using SafeMath in places where they are direct double checks (in all places except deposit, where using SafeMath might be justified if we assume that token contract could be compromised), so we already spend extra gas for nothing. I really don't see the reason why you want to push it to an extreme.