Add off-chain signature generation and on-chain verification #7
Conversation
There was a problem hiding this comment.
Sorry for the delay - was in the air.
This is really great work, thank you! Have left some comments (mostly just questions from someone still on the learning curve ;) ).
One thing I am still struggling a little with:
Running tests the first time failed at the last one:
Contract: NonFungibleToken issuing signature validation when the attacker tries to use a signature generated by a party other than the `collectableCouponIssuer` reverts:
AssertionError: Expected "revert", got AssertionError: assert.fail() instead
and then worked perfectly when I tried to run it again a few seconds later. Probably just some local weirdness, but any idea what might have caused that? So strange...
Anyway, like I say, great work! Now to think about next steps... How to submit the signed transaction to the network (happy with just using infura rather than requiring running a full geth node so that we can keep the backend super light and not require a more expensive VM) and show the user the artwork associated with that token on their phone in an awesome and artistic kind of way...
| @@ -11,6 +11,8 @@ | |||
| "babel-loader": "6.2.7", | |||
| "babel-preset-react-app": "^2.0.1", | |||
| "case-sensitive-paths-webpack-plugin": "1.1.4", | |||
| "chai-as-promised": "^7.1.1", | |||
| "chai-bignumber": "^2.0.2", | |||
| "chalk": "1.1.3", | |||
| "connect-history-api-fallback": "1.3.0", | |||
| "cross-spawn": "4.0.2", | |||
There was a problem hiding this comment.
Please add
"ethereumjs-abi": "^0.6.5",
to the package.json - your choice as to whether in dependencies or devDependencies. Would be inclined to go with the second if I were to have my way seeing as it is only required in the test suite.
There was a problem hiding this comment.
Ah, ok, I'll completely remove that, it isn't necessary in any of the tests yet, if it becomes necessary then I'll add it again.
contracts/NonFungibleCollectable.sol
Outdated
| } | ||
|
|
||
| function getAddress(bytes32 hash, uint8 v, bytes32 r, bytes32 s) public pure returns(address) { | ||
| bytes memory prefix = "\x19Ethereum Signed Message:\n32"; |
There was a problem hiding this comment.
Would you mind leaving a few comments in this file? In particular, could you explain the need to have prefix in memory rather than storage as well as the form of the prefix you have gone with here? What exactly is going on there?
There was a problem hiding this comment.
Very good point! Haha, this should be done off chain! :)
There was a problem hiding this comment.
👍 cool, was thinking that there was something else weird and new I'd need to learn about :/n32 and stuff 😅
There was a problem hiding this comment.
Hmm, very wierd the hash I'm getting by computing this off chain is different, I'll get it right though, I hope 😅
test/NonFungibleCollectable.test.js
Outdated
| newOwner.should.be.equal(recipientAddress) | ||
|
|
||
| const {hash, v, r, s} = signatureObject | ||
| await assertRevert(token.claimCollectableWithCoupon(_firstTokenId, hash, v, r, s, {from: recipientAddress})) |
There was a problem hiding this comment.
This test would make more sense to me if it were {from: attackerAddress}. As it currently stands, my understanding is that this reverts because it calls NonFungibleCollectable at L40 and reverts due to NonFungibleToken at L235 (i.e. you have transferred the token to the recipientAddress and having the recipient try to claim a token they own already reverts...) BTW changing to attackerAddress also reverts as expected.
There was a problem hiding this comment.
Ok, sure, I can change that :)
|
No problem! :) Yes, I actually don't know why it does that (the revert vs assert error), it must be some confusion in ganache, because I've also noticed it as a transient issue, maybe I'll just change the 'assertRevert' code to accept assert errors also (basically we just want to know that the smart contract isn't processing it). I'm not sure what you mean by 'How to submit the signed transaction to the network', we'll use the provider in Status won't we? This is the transaction that the user will execute on their device (the signature is just the proof that the server gives its approval). We will though need to make sure the server isn't compromised, but that is a given. |
This is why you should not review code at midnight 😅 Absolutely right, my bad. |
…aning more explicit - remove unused variable
|
Should have fixed up the lose ends above |
test/NonFungibleCollectable.test.js
Outdated
| const {hash, v, r, s} = signatureObject | ||
| await assertRevert(token.claimCollectableWithCoupon(_secondTokenId, hash, v, r, s, {from: recipientAddress})) | ||
| const {prefixedHash, v, r, s} = signatureObject | ||
| await assertRevert(token.claimCollectableWithCoupon(_secondTokenId, prefixedHash, v, r, s, {from: recipientAddress})) | ||
| const newOwner = await token.ownerOf(_firstTokenId) |
There was a problem hiding this comment.
I think this should be const newOwner = await token.ownerOf(_secondTokenId) in order to agree with trying to claim the token that has not been assigned by a valid coupon. Changing it still gets the expected revert().
There was a problem hiding this comment.
Yes, agreed, but since we are using the signature for the 1st token, it makes sense to test both tokens actually
|
Btw, I'll keep working on this for the next while at this sort of pace, and I'll try give you accurate predictions when I'll get done, just so you can stay on top of progress :) |
Fixes #6