File split and removing solc warnings #12

mmhh1910 · 2018-03-09T07:21:32Z

Splitted contracts into separate files and contract subdirectory (as a preparation for truffle project setup)
Updated to current SafeMath library
Replaces call.value with transfer
Adjusted all pragmas to current solc 0.4.21
Added emit keyword for all event calls
Fixed stack to deep warning by using an array
Minor code formatting

Updated to SafeMath library

Fixing stack too deep warning Adding emit for events

JonathonDunford · 2018-03-09T15:00:42Z

@betterdelta How is this going to work when deployed on the blockchain? How do you utilize multiple smart contracts on the blockchain?

mmhh1910 · 2018-03-09T15:27:30Z

Just like it worked with the ED contract. The two contracts at the top just moved to their own file and an import statement for both of these files was added to the main file.

Technically, the imports will work just as if the contents of the file was in the main ForkDelta.sol file instead of the import lines.

JonathonDunford · 2018-03-09T20:22:09Z

@betterdelta So you would push all three contracts to the blockchain and link them that way?

I am moreso asking how this is going to work when deployed.

mmhh1910 · 2018-03-09T21:18:13Z

All contracts will be compiled with solc and deployed to the chain with truffle. The FD contract will know about the contract/library defintions in the imported files.

I can't explain it on a technical level. It's the magic of the solc compiler that links these deployed contracts/libraries together in bytecode.

I think http://solidity.readthedocs.io/en/develop/using-the-compiler.html has some info how linking works with solc.

JonathonDunford

Great work so far. Several things.

JonathonDunford · 2018-03-11T14:29:37Z

contracts/SafeMath.sol

+  * @dev Multiplies two numbers, throws on overflow.
+  */
+  function mul(uint256 a, uint256 b) internal pure returns (uint256) {
+    if (a == 0) {


Should the logic be if a == 0 OR b == 0 ?

SafeMath.sol is from https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/math/SafeMath.sol

They kind of know what they are doing, at least much better than I do.

Regarding your comment, no, for the check that follows it's only vital that a is not 0.

Well, if either A or B is 0, the function will return 0.

Yes, but if a is 0 the function would in fact throw an division by zero exception. In contrast to b = 0.

I guess that's true.

JonathonDunford · 2018-03-11T14:30:59Z

contracts/SafeMath.sol

+  * @dev Integer division of two numbers, truncating the quotient.
+  */
+  function div(uint256 a, uint256 b) internal pure returns (uint256) {
+    // assert(b > 0); // Solidity automatically throws when dividing by 0


We don't want a throw; here do we?
Assert/require/revert would probably be better? Or does that comment mean it will revert?

Yes, Solidity will automatically throw/revert on division by zero.

Does it revert or does it throw? Very different outcomes.

From the solidity docs:

An assert-style exception is generated in the following situations:
...
If you divide or modulo by zero (e.g. 5 / 0 or 23 % 0).

So, the OpenZeppelin removed the assert, because it is implicitly done by solidity already.

I think it would be better to revert? Save users gas if you can?

JonathonDunford · 2018-03-11T14:31:56Z

contracts/SafeMath.sol

+  */
+  function add(uint256 a, uint256 b) internal pure returns (uint256) {
+    uint256 c = a + b;
+    assert(c >= a);


Do you think the contract should revert if you add by 0?

We should be using require everywhere instead of assert AFAIK.

"assert(false) compiles to 0xfe, which is an invalid opcode, using up all remaining gas, and reverting all changes.

require(false) compiles to 0xfd which is the REVERT opcode, meaning it will refund the remaining gas. The opcode can also return a value (useful for debugging), but I don't believe that is supported in Solidity as of this moment. (2017-11-21)"

assert(c >= a); will revert if you try to add something by 0 as it is assuming the next value will be larger. Not sure this is behavior we want?

@betterdelta

Thoughts on this?

Re 1.): You are in opposition to https://solidity.readthedocs.io/en/develop/control-structures.html#error-handling-assert-require-revert-and-exceptions:

"The assert function should only be used to test for internal errors, and to check invariants. The require function should be used to ensure valid conditions, such as inputs, or contract state variables are met, or to validate return values from calls to external contracts. If used properly, analysis tools can evaluate your contract to identify the conditions and function calls which will reach a failing assert. Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix."

Re 2:
a.) Assert will throw an exception, not revert.
b.) I don't get it. Neither 10.add(0), now 0.add(10), nor 0,add(0) will make the assert statement throw an exception.

I understand it's in opposition to their proposed structure, however doesn't it provides a better user experience to revert transactions and return gas instead of tipping the miners extra?

I was reading that wrong. I guess it just doesn't support addition by negatives.

JonathonDunford · 2018-03-11T14:32:31Z

contracts/ForkDelta.sol

-  function transferFrom(address _from, address _to, uint256 _value) public returns (bool success) {}
+contract ForkDelta {
+
+  using SafeMath for uint;


JonathonDunford · 2018-03-11T14:36:25Z

contracts/ForkDelta.sol

-    require(msg.sender.call.value(amount)());
-    Withdraw(0, msg.sender, amount, tokens[0][msg.sender]);
+    tokens[0][msg.sender] = tokens[0][msg.sender].sub(amount);
+    msg.sender.transfer(amount);


Why did we remove the require here?
Why did we change call.value to .transfer throughout?

Previously: require throws when function returns false, e.g. when not enough eth.
New version: transfer will not return a result, but throw on fail itself, so no require needed

You could also say that transfer is safer to use, because you don't have to check the result.

Reason for the change: Safety against reentrancy as explained in:

https://consensys.github.io/smart-contract-best-practices/recommendations/#be-aware-of-the-tradeoffs-between-send-transfer-and-callvalue

JonathonDunford · 2018-03-11T14:37:34Z

contracts/ForkDelta.sol

@@ -115,31 +63,31 @@ contract ForkDelta is SafeMath {
  }

  function deposit() public payable {
-    tokens[0][msg.sender] = safeAdd(tokens[0][msg.sender], msg.value);
-    Deposit(0, msg.sender, msg.value, tokens[0][msg.sender]);
+    tokens[0][msg.sender] = tokens[0][msg.sender].add(msg.value);


So much more syntactically pleasing. Good addition.

JonathonDunford · 2018-03-11T14:40:04Z

contracts/ForkDelta.sol

@@ -176,35 +124,45 @@ contract ForkDelta is SafeMath {
    uint feeMakeXfer = 0;
    uint feeTakeXfer = 0;
    if (now >= freeUntilDate) {
-      feeMakeXfer = safeMul(amount, feeMake) / (1 ether);
-      feeTakeXfer = safeMul(amount, feeTake) / (1 ether);
+      feeMakeXfer = amount.mul(feeMake) / (1 ether);


Do we need a SafeMath division?

No. Dividing by 1 ether will never result in an overflow.

JonathonDunford · 2018-03-11T14:42:26Z

contracts/ForkDelta.sol

-    tokens[tokenGive][msg.sender] = safeAdd(tokens[tokenGive][msg.sender], safeMul(amountGive, amount) / amountGet);
+    tokens[tokenGet][msg.sender] = tokens[tokenGet][msg.sender].sub(amount.add(feeTakeXfer));
+    tokens[tokenGet][user] = tokens[tokenGet][user].add(amount.sub(feeMakeXfer));
+    tokens[tokenGet][feeAccount] = tokens[tokenGet][feeAccount].add(feeMakeXfer.sub(feeTakeXfer));


Whoops, typo:

feeMakeXfer.sub(feeTakeXfer)

Should be

feeMakeXfer.add(feeTakeXfer)

Ups.. Will fix.

JonathonDunford · 2018-03-11T14:43:50Z

contracts/ForkDelta.sol

+    tokens[tokenGet][user] = tokens[tokenGet][user].add(amount.sub(feeMakeXfer));
+    tokens[tokenGet][feeAccount] = tokens[tokenGet][feeAccount].add(feeMakeXfer.sub(feeTakeXfer));
+    tokens[tokenGive][user] = tokens[tokenGive][user].sub(amountGive.mul(amount) / amountGet);
+    tokens[tokenGive][user] = tokens[tokenGive][user].sub(amountGive.mul(amount) / amountGet);


Duplicate line.

Ups. Will fix.

JonathonDunford · 2018-03-11T14:51:58Z

contracts/ForkDelta.sol

+    uint[2] memory available;
+    available[0] = amountGet.sub(orderFills[user][hash]);
+    available[1] = tokens[tokenGive][user].mul(amountGet) / amountGive;
+    if (available[0]<available[1]) {


Let's make this (available[0] < available[1]) for code formatting consistency.

Replaced one multiplication with SafeMath.mul Minor formatting

JonathonDunford · 2018-03-12T21:40:27Z

My thoughts:

Basically, require will check something, then throw a REVERT code if it fails.
Assert will check, but throw an invalid opcode.

Assert eats up all of the remaining gas while Require returns the remaining gas to the user.

Official spec documentation for solidity states that assert should never happen, but to use assert to "test for internal errors"

I understand the reasoning for that when testing contracts and developing something you can change, but I don't really agree with that in a live environment that can't be changed easily.

My suggestion is that we use require or revert instead of assert and throw. My reasoning is that it is better for users to return gas if there ever is an error (like a malformed integer breaking a safeMath equation).

Using assert IMO is basically for testing purposes (as confirmed in their docs). Currently it's only used in our safeMath library.

I'm guessing it could reach the current asserts in safeMath if users try to directly interface with contract and put the wrong values in.

JonathonDunford · 2018-03-12T21:58:15Z

ethereum/solidity#1130
ethereum/solidity#1678

It really seems like their original intent was NOT for assert to be in production code.

Discussion in #12 Added our standard .gitignore

JonathonDunford · 2018-03-12T22:09:16Z

I made the change from assert to require.

mmhh1910 · 2018-03-13T07:11:21Z

I am strongly against changing assert to require and here are my comments:

1.) I have a strong opinion on this, but I'll try to relax, because I think we will see this one again in the external audit.

2.) My main headache is not so much about what technically happens when we use revert instead of assert, but the attitude and the lack of humility that comes with this change.
The SafeMath.sol library is in like every second contract (with the assert statement) and it was written by and reviewed by a lot of knowledgable developers as part of the OpenZeppelin project ("OpenZeppelin is a library for writing secure Smart Contracts on Ethereum.").
Further, the solidity documentation clearly states that assert should be used for this kind of overflow prevention.
Are we in a position to say "we know better"? And if so, wouldn't the correct way be to head over to https://github.com/OpenZeppelin/zeppelin-solidity and send a PR for the change from assert to require? Once it went through the peer review into their master, I am all for copying that change (aka keeping the external library that we use up to date).
For me this feels like coin devs implemeting their own cryptographic functions, because they know better.

3.) Expanding on this, it has been said: "So if we follow along with <Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.> Then, we shouldn't have assert there."
IMHO asserts should be in production code, especially with smart contracts (user funds involved and no way to fix code). They are our last resort in case we made a mistake. Most of us are human, so we will.

4.) I don't see any benefit for using revert over assert. Our hope is that the assert statements will never throw an exception, even to an extend that it was proposed to remove the assert statements at all. Thus, I can't see why it should be plus on the revert side that it does not eat gas. It never happens anyway...
Seriously, if we frequently run into asserts and significant amounts of gas is are burned, our problem is not the burned gas, but a contract that should be replaced.

JonathonDunford · 2018-03-13T15:21:32Z

@betterdelta

I understand your arguments for, but I think the benefit is clear. When users malform input to the contract, they will get gas returned instead of making a donation to miners.

In the above statement, it is assumed that assert will never be hit.

The fact of the matter is it will be hit because we have no control over what people pass into the contract.

So, in the case that the code is hit, what do you think would be the best outcome?

I understand these are protocols that other people put in place, but protocols are changed and updated by developers like ourselves. Even the Bible and Constitution have been updated.

This is what it looks like to me:

Pros:

It returns gas if users mess up (which they will)

Cons:

It isn't exactly what OpenZeppelin made

Solidity docs are conflicting in their "recommendations"

Barring other user's input, would you say we could leave this to the official audit and let the community / official auditors make suggestions?

I do honestly understand where you are coming from.

mmhh1910 · 2018-03-14T07:01:06Z

Two more links on this:

Your change in a denied PR at OpenZeppelin just a week ago: OpenZeppelin/openzeppelin-contracts#778

ConsenSys Diligence "Ethereum Smart Contract Security Best Practices" on the topic:
https://consensys.github.io/smart-contract-best-practices/recommendations/#use-assert-and-require-properly

I'll take this as a quality test for our audits. IMHO, if this is still in after the audits, we had the wrong audits. :-)

JonathonDunford · 2018-03-14T14:57:02Z

@betterdelta

That PR shows the solidity docs preferring require in these situations.

OZ's response:
"Those asserts are invariants, and follow the documentation correctly :)"

Are they still "invariants" if someone can pass any data they want to the contract and math WILL be done on it?

If you want assert() in SafeMath, the ForkDelta contract should have many more require() checks before it does math on anything, so that the asserts would never, ever be called.

mmhh1910 added 4 commits March 8, 2018 22:25

Split into separate contract files and contracts subdirectory

4e494fa

Updated to SafeMath library

Replaced call.value with transfer

b0e9609

Fixed typo, adjusted pragmas

d92f6e4

Fixing stack too deep warning Adding emit for events

Code formatting

5d136e7

mmhh1910 requested a review from JonathonDunford March 9, 2018 07:33

forkdelta deleted a comment from mmhh1910 Mar 9, 2018

JonathonDunford reviewed Mar 11, 2018

View reviewed changes

mmhh1910 added 2 commits March 11, 2018 22:08

Removed dupe line

41d9239

Fixes

6b8f429

mmhh1910 added 2 commits March 12, 2018 15:51

Merged changes.

e6e46e9

Replaced / with div for consistency

3f2684e

Replaced one multiplication with SafeMath.mul Minor formatting

Change assert to require in SafeMath.

08068fe

Discussion in #12 Added our standard .gitignore

JonathonDunford merged commit 7a80575 into master Mar 13, 2018

JonathonDunford deleted the file_split branch March 13, 2018 00:04

File split and removing solc warnings #12

File split and removing solc warnings #12

Conversation

mmhh1910 commented Mar 9, 2018 • edited Loading

JonathonDunford commented Mar 9, 2018

mmhh1910 commented Mar 9, 2018

JonathonDunford commented Mar 9, 2018

mmhh1910 commented Mar 9, 2018

JonathonDunford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonathonDunford Mar 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmhh1910 Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonathonDunford Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonathonDunford commented Mar 12, 2018

JonathonDunford commented Mar 12, 2018

JonathonDunford commented Mar 12, 2018

mmhh1910 commented Mar 13, 2018

JonathonDunford commented Mar 13, 2018 • edited Loading

mmhh1910 commented Mar 14, 2018

JonathonDunford commented Mar 14, 2018

mmhh1910 commented Mar 9, 2018 •

edited

Loading

JonathonDunford Mar 11, 2018 •

edited

Loading

mmhh1910 Mar 12, 2018 •

edited

Loading

JonathonDunford Mar 12, 2018 •

edited

Loading

JonathonDunford commented Mar 13, 2018 •

edited

Loading