Contract #2

Nitr4x · 2019-07-16T15:36:02Z

No description provided.

xavierlepretre · 2019-07-18T13:10:32Z

contracts/Remittance.sol

+    function newTransaction(address exchange, bytes32 password) _onlyIfRunning public payable returns(bool success) {
+        require(exchange != address(0) && password != 0 && msg.value > 0, "An error occured. Ensure that both exchange and password are set and that your transaction value is above 0");
+
+        bytes32 hash = hashOTP(exchange, password);


You know that because of how consensus works:

all transactions are public

all transaction data is public

all code execution is public

What does it mean for your password?

Indeed, the password can be read in cleartext. However, to be able to retrieve the transaction value, the call must be performed from the exchange wallet. What is the point in your statement?

No web3.eth.getTransaction(hash). Anyone can do that. Only access to a synchronised node is required.

What I mean by transaction value is the possibility to withdraw the ether in the contract. What are you suggesting?

For the exercise, Alice sends Ether for Carol to withdraw and give the $ to Bob.
We want Carol to wait for Bob and his secret to be able to withdraw.
In your setup, Carol does not need to wait because the secret is already there. Carol can withdraw without Bob's secret (not a secret).
You have to make it so that Carol has to wait for Bob for the secret.

xavierlepretre · 2019-07-18T13:11:22Z

contracts/Remittance.sol

+    }
+
+    function withdrawFees() public _onlyOwner returns(bool success) {
+        uint amount = fees;


Who owns those != 0 fees after an owner change?

xavierlepretre · 2019-07-18T13:12:23Z

contracts/Remittance.sol

+    }
+
+    function withdraw(bytes32 password) public returns(bool success) {
+        require(password != 0, "Password must be set");


This check is not terribly important. After all, if it is wrong, it will fail on line 77.
So if you remove it, it will be cheaper for successful withdraws, and more expensive for mistakes.

xavierlepretre · 2019-07-18T13:13:02Z

contracts/Remittance.sol

+
+        require(tx.amount > 0, "Neither the transaction does not exist or the password is wrong");
+        require(now >= tx.time, "You must wait 5 days before cancelling the transaction");
+        require(tx.emitter == msg.sender, "The call must be initiated by the wanted exchange");


Because of this check, it is probably not necessary to go through the hash calculation. Just passing the hash should be enough.

"by the emitter"?

xavierlepretre · 2019-07-18T13:13:15Z

contracts/Remittance.sol

+contract Remittance is Stoppable {
+    using SafeMath for uint;
+
+    struct Transaction {


Because transaction has a meaning in Ethereum, perhaps another name?

xavierlepretre · 2019-07-18T13:13:40Z

contracts/Remittance.sol

+        }
+
+        emit LogNewTransaction(msg.sender, exchange, amount);
+        transactions[hash] = Transaction({time: now.add(CANCELLATION_DELAY), emitter: msg.sender, amount: amount});


Would look nicer over more lines.

xavierlepretre · 2019-07-18T13:15:29Z

contracts/Remittance.sol

+        require(transactions[hash].emitter == address(0), "Password already used");
+
+        uint amount = msg.value;
+        if (getOwner() != msg.sender) {


That is an expensive check. I would say, you can always take fees, even from the owner.

According to the project guidance : make you, the owner of the contract, take a cut of the Ethers. How much? Your call. Perhaps smaller than what it would cost Alice to deploy the same contract herself

What I mean is:

fees = fees.add(TX_FEES); uint amount = msg.value.sub(TX_FEES); emit LogTakeFees(msg.sender, TX_FEES);

No getOwner() != msg.sender.

xavierlepretre · 2019-07-19T14:52:16Z

contracts/Remittance.sol

-        }
+
+        fees[getOwner()] = fees[getOwner()].add(TX_FEES);
+        amount = amount.sub(TX_FEES);


I think you can save gas by replacing:

uint amount = msg.value; amount = amount.sub(TX_FEES);

with:

uint amount = msg.value.sub(TX_FEES);

Well spotted!

xavierlepretre · 2019-07-19T14:54:16Z

contracts/Remittance.sol

-    mapping(bytes32 => Transaction) private transactions;
-    uint private fees;
+    mapping(bytes32 => Order) private transactions;
+    mapping(address => uint) private fees;


Are you intentionally making it difficult for people to know the status of the contract?

I intentionally wanted to make these variables private to ensure confidentiality

Do you know that all data is public? There is no confidentiality. All you are doing is asking people to do a sha3 and a web3.eth.getStorageAt to get the data.
The only people who will manage that are potential attackers.
Best practice: make all data public.

Gotcha this time. Quite difficult to admit that everything is public and tracable!

xavierlepretre · 2019-07-19T14:54:55Z

contracts/Remittance.sol

            amount = amount.sub(TX_FEES);
            emit LogTakeFees(msg.sender, TX_FEES);
        }

        emit LogNewTransaction(msg.sender, exchange, amount);
-        transactions[hash] = Transaction({time: now.add(CANCELLATION_DELAY), emitter: msg.sender, amount: amount});
+        transactions[hashedOTP] = Order({
+            time: now.add(CANCELLATION_DELAY),


Cancellation delay could be a parameter passed by the sender. Within reason.

xavierlepretre · 2019-07-19T14:56:06Z

contracts/Remittance.sol

-        transactions[hash].emitter = address(0);
+        transactions[hashedOTP].amount = 0;
+        transactions[hashedOTP].time = 0;
+        transactions[hashedOTP].emitter = address(0);


You could say delete transactions[hashedOTP].

Are you intentionally allowing reuse of previously used passwords?

I forgot this utility ;)

xavierlepretre · 2019-07-19T14:57:07Z

contracts/Remittance.sol


        require(amount > 0, "Nothing to withdraw");

-        fees = 0;
+        fees[getOwner()] = 0;


You could save owner in memory and do SLOAD only once.

xavierlepretre · 2019-07-19T14:57:46Z

contracts/Remittance.sol

@@ -87,11 +86,11 @@ contract Remittance is Stoppable {
    }

    function withdrawFees() public _onlyOwner returns(bool success) {


_onlyOwner, so if I am a former owner, I cannot withdraw?

I thought the point of this mapping was that if I was a former owner, I could withdraw my fees after the ownership transfer without having to time my withdraw and ownerChange transactions so that they are close to each other.

I forgot to delete the modifier when replacing the uint by a mapping

xavierlepretre · 2019-07-19T16:20:48Z

contracts/Remittance.sol

        emit LogTakeFees(msg.sender, TX_FEES);

        emit LogNewTransaction(msg.sender, exchange, amount);
        transactions[hashedOTP] = Order({
-            time: now.add(CANCELLATION_DELAY),
+            time: now.add((delay == 0) ? CANCELLATION_DELAY : delay),


Avoid fixing user mistakes. You don't like the delay? -> revert.

xavierlepretre · 2019-07-19T16:21:31Z

contracts/Remittance.sol

-        uint amount = fees[getOwner()];
+    function withdrawFees() public returns(bool success) {
+        address owner = getOwner();
+        uint amount = fees[owner];


So only the current owner can withdraw?
If I am the former owner, and I still have something to withdraw, how do I do?

xavierlepretre · 2019-07-19T23:20:01Z

contracts/Remittance.sol

+    using SafeMath for uint;
+
+    struct Order {
+        uint time;


deadline to be more expressive?

xavierlepretre · 2019-07-19T23:20:25Z

contracts/Remittance.sol

+
+    uint constant TX_FEES = 2000;
+
+    mapping(bytes32 => Order) public transactions;


Can you rename to orders too?

xavierlepretre · 2019-07-19T23:20:35Z

contracts/Remittance.sol

+    mapping(bytes32 => Order) public transactions;
+    mapping(address => uint) public fees;
+
+    event LogNewTransaction(address indexed emitter, address exchange, uint amount);


LogNewOrder?

I think all your events should mention the hashed OTP. After all, that's the only thing that helps us follow the lifecycle of an order.

Agree. All excepting LogTakeFes :)

xavierlepretre · 2019-07-19T23:20:42Z

contracts/Remittance.sol

+
+    event LogNewTransaction(address indexed emitter, address exchange, uint amount);
+    event LogTakeFees(address indexed emitter, uint amount);
+    event LogCancelTransaction(address indexed emitter, address exchange, uint amount);


LogCancelOrder?

xavierlepretre · 2019-07-19T23:22:59Z

contracts/Remittance.sol

+        fees[getOwner()] = fees[getOwner()].add(TX_FEES);
+        emit LogTakeFees(msg.sender, TX_FEES);
+
+        emit LogNewTransaction(msg.sender, exchange, amount);


You notice that exchange here is purely informative. There is no enforcement to ensure its validity.
Perhaps rename it to express that?

Deleting exchange.

xavierlepretre · 2019-07-19T23:24:56Z

contracts/Remittance.sol

+
+        transactions[hash].amount = 0;
+        transactions[hash].time = 0;
+        transactions[hash].emitter = address(0);


Not sure, do you want to let emitters reuse the same password?

Well it's up to them but I don't want to restrict password reuse. Except if an undergoing order is still waiting to be withdawed with the same password / exhange address.

What happens if Alice reuses a password.
Carol does not need to wait for Bob to come with the password. Even better, if Alice gave the same password to Dan, Carol does not need to wait for Dan to come.
Carol can just withdraw and leave. Alice will not be happy to know that she, or her front-end, has to remember all the passwords she has used in the past.
In this setup, you have to assume that Alice is the less tech savvy person.

xavierlepretre · 2019-07-19T23:26:04Z

contracts/Remittance.sol

+
+        delete(transactions[hashedOTP]);
+
+        emit LogCancelTransaction(msg.sender, exchange, tx.amount);


Here too exchange is purely informative.

And falsifiable.

Will remove the exchange, in fact the exchange does not need to be pinged if an order is placed

xavierlepretre · 2019-07-19T23:26:30Z

contracts/Stoppable.sol

+
+contract Stoppable is Owned {
+
+    bool isRunning;


Are you inviting the developer of the child contract to change the value at will?

Indeed, I prefer this design pattern than a real kill switch

What I mean, is which way do you want the developer of the child contract to use:

isRunning = false;

or

pauseContract();

One emits an event and controls for ownership, the other doesn't.

I meant bool private isRunning;

xavierlepretre · 2019-07-19T23:26:38Z

contracts/Stoppable.sol

+    bool isRunning;
+
+    event LogPausedContract(address sender);
+    event LogResumeContract(address sender);


indexed perhaps?

xavierlepretre · 2019-07-19T23:26:52Z

contracts/Stoppable.sol

+    }
+
+    constructor() public {
+        isRunning = true;


Can you make this initial value a constructor parameter?

xavierlepretre · 2019-07-22T09:08:59Z

contracts/Remittance.sol

+    event LogNewOrder(address indexed emitter, uint amount, bytes32 hashedOTP);
+    event LogTakeFees(address indexed emitter, uint amount);
+    event LogCancelOrder(address indexed emitter, uint amount, bytes32 hashedOTP);
+    event LogWithdraw(address indexed emitter, uint amount);


hashedOTP?

Also, do you think people will filter per hashedOTP from the client side?

To track undergoing transfers yes, I think.

xavierlepretre · 2019-07-22T09:11:07Z

contracts/Remittance.sol

+        address owner = getOwner();
+
+        fees[owner] = fees[owner].add(TX_FEES);
+        emit LogTakeFees(msg.sender, TX_FEES);


It could be interesting to inform the owner too.

xavierlepretre · 2019-07-22T09:11:46Z

contracts/Remittance.sol

+
+        emit LogNewOrder(msg.sender, amount, hashedOTP);
+        orders[hashedOTP] = Order({
+            deadline: now.add(delay),


We did not discuss it but you have to know that the timestamp is not very accurate, but good enough for this use case.

xavierlepretre · 2019-07-22T09:12:34Z

contracts/Remittance.sol

+    }
+
+    function cancelOrder(bytes32 hashedOTP) public returns(bool success) {
+        require(hashedOTP != 0, "The secret is not set");


This is not necessary as there is no order at 0. See line 33.

xavierlepretre · 2019-07-22T09:13:32Z

contracts/Remittance.sol

+
+        require(tx.amount > 0, "Neither the transaction does not exist or the password is wrong");
+        require(now >= tx.time, "You must wait 5 days before cancelling the transaction");
+        require(tx.emitter == msg.sender, "The call must be initiated by the wanted exchange");


"by the emitter"?

xavierlepretre · 2019-07-22T09:14:01Z

contracts/Remittance.sol

+
+        orders[hashedOTP].amount = 0;
+        orders[hashedOTP].deadline = 0;
+        orders[hashedOTP].emitter = address(0);


delete orders[hashedOTP];

xavierlepretre · 2019-07-22T09:14:53Z

contracts/Remittance.sol

+    mapping(bytes32 => Order) public orders;
+    mapping(address => uint) public fees;
+
+    event LogNewOrder(address indexed emitter, uint amount, bytes32 hashedOTP);


It would be good to have the events be a verb past tense. LogNewOrderPlaced, LogWithdrawn, LogOrderCancelled, LogFeesTaken.

…t creation

…o detect the password associated to a given hash

xavierlepretre · 2019-07-25T17:49:55Z

contracts/Remittance.sol

+        require(now >= tx.deadline, "You must wait 5 days before cancelling the transaction");
+        require(tx.emitter == msg.sender, "The call must be initiated by the emitter");
+
+        delete(orders[hashedOTP]);


I think that even if it was cancelled, the password may have been disclosed to Carol. So I think it still would not be safe to allow reuse.

Patch in one my latest commit :)

xavierlepretre · 2019-07-25T17:52:28Z

contracts/Remittance.sol

+
+    function newOrder(bytes32 hashedOTP, uint delay) _onlyIfRunning public payable returns(bool success) {
+        require(hashedOTP != 0 && msg.value > 0, "An error occured. Ensure that the secret is set and that your transaction value is above 0");
+        require(delay > 0 days, "Delay should be above 0 day");


I think > 0 is the same, cheaper.
Also, on line 44, it is used as seconds. Which is good. You don't want conversions to be handled in the EVM.

Nitr4x · 2019-07-25T19:21:16Z

Too much comments. Creating new pull request.

Nitr4x added 3 commits July 16, 2019 14:18

Pushing contracts

b3e02b7

Adding remix env

7b2fd0a

Fixing typo

a57bd00

Nitr4x mentioned this pull request Jul 16, 2019

Test #1

Closed

xavierlepretre reviewed Jul 18, 2019

View reviewed changes

Nitr4x added 2 commits July 19, 2019 16:36

Updating contract

55a8878

Changing fees logic

89ff990

xavierlepretre reviewed Jul 19, 2019

View reviewed changes

Update contracts

05e2a09

xavierlepretre reviewed Jul 19, 2019

View reviewed changes

Updating contract according to feedback

4130e6b

xavierlepretre reviewed Jul 19, 2019

View reviewed changes

Modifying contract according to feedback

46b5516

xavierlepretre reviewed Jul 22, 2019

View reviewed changes

Nitr4x added 3 commits July 22, 2019 19:49

Applying small changes

5a0f1e8

Passing the isrunning state as private and rollback Stoppable contrac…

1e51efa

…t creation

Modifying the way to delete an order when withdrawing it to prevent t…

aa362c8

…o detect the password associated to a given hash

xavierlepretre reviewed Jul 25, 2019

View reviewed changes

Nitr4x added 4 commits July 25, 2019 20:12

Adding killswitch contract

71dc2ea

Modifying killable contract variable name

7f5cb9f

Updating Killable contract

0f32847

Small change in the remittance contract

147fc04

Nitr4x closed this Jul 25, 2019

		@@ -87,11 +86,11 @@ contract Remittance is Stoppable {
		}

		function withdrawFees() public _onlyOwner returns(bool success) {


		uint constant TX_FEES = 2000;

		mapping(bytes32 => Order) public transactions;


		delete(transactions[hashedOTP]);

		emit LogCancelTransaction(msg.sender, exchange, tx.amount);

Contract #2

Contract #2

Conversation

Nitr4x commented Jul 16, 2019

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

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

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

Nitr4x Jul 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nitr4x Jul 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nitr4x Jul 21, 2019 • 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

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

Nitr4x commented Jul 25, 2019

Nitr4x Jul 21, 2019 •

edited

Loading

Nitr4x Jul 21, 2019 •

edited

Loading

Nitr4x Jul 21, 2019 •

edited

Loading