From 8f5ecc763bb75a30c2be28f28bbeefb8ad454239 Mon Sep 17 00:00:00 2001 From: Ralph Pichler Date: Mon, 22 Jul 2019 17:34:59 +0200 Subject: [PATCH 1/4] swap: compute actual payment amount and verify honey amount and serial --- swap/peer.go | 94 ++++++++++++++++++++++++++++++++++++++--------- swap/swap.go | 16 ++++++++ swap/swap_test.go | 44 ++++++++++++++++++++++ 3 files changed, 137 insertions(+), 17 deletions(-) diff --git a/swap/peer.go b/swap/peer.go index 31de9c3d67..2018dcb1bb 100644 --- a/swap/peer.go +++ b/swap/peer.go @@ -36,10 +36,11 @@ var ErrDontOwe = errors.New("no negative balance") // Peer is a devp2p peer for the Swap protocol type Peer struct { *protocols.Peer - swap *Swap - backend cswap.Backend - beneficiary common.Address - contractAddress common.Address + swap *Swap + backend cswap.Backend + beneficiary common.Address + contractAddress common.Address + lastReceivedCheque *Cheque } // NewPeer creates a new swap Peer instance @@ -76,25 +77,20 @@ func (sp *Peer) handleEmitChequeMsg(ctx context.Context, msg *EmitChequeMsg) err log.Info("received emit cheque message") cheque := msg.Cheque - if cheque.Contract != sp.contractAddress { - return fmt.Errorf("wrong cheque parameters: expected contract: %s, was: %s", sp.contractAddress, cheque.Contract) - } - - // the beneficiary is the owner of the counterparty swap contract - if err := sp.swap.verifyChequeSig(cheque, sp.beneficiary); err != nil { - log.Error("error invalid cheque", "from", sp.ID().String(), "err", err.Error()) + if err := sp.verifyChequeProperties(msg.Cheque); err != nil { return err } - if cheque.Beneficiary != sp.swap.owner.address { - return fmt.Errorf("wrong cheque parameters: expected beneficiary: %s, was: %s", sp.swap.owner.address, cheque.Beneficiary) - } + lastCheque := sp.loadLastReceivedCheque() - if cheque.Timeout != 0 { - return fmt.Errorf("wrong cheque parameters: expected timeout to be 0, was: %d", cheque.Timeout) + if err := sp.verifyChequeAgainstLast(msg.Cheque, lastCheque); err != nil { + return err } - // TODO: check serial and balance are higher + if err := sp.saveLastReceivedCheque(cheque); err != nil { + log.Error("error while saving last received cheque", "peer", sp.ID().String(), "err", err.Error()) + // TODO: what do we do here? + } // reset balance by amount // as this is done by the creditor, receiving the cheque, the amount should be negative, @@ -137,3 +133,67 @@ func (sp *Peer) handleErrorMsg(ctx context.Context, msg interface{}) error { // maybe balance disagreement return nil } + +func (sp *Peer) verifyChequeProperties(cheque *Cheque) error { + if cheque.Contract != sp.contractAddress { + return fmt.Errorf("wrong cheque parameters: expected contract: %s, was: %s", sp.contractAddress, cheque.Contract) + } + + // the beneficiary is the owner of the counterparty swap contract + if err := sp.swap.verifyChequeSig(cheque, sp.beneficiary); err != nil { + log.Error("error invalid cheque", "from", sp.ID().String(), "err", err.Error()) + return err + } + + if cheque.Beneficiary != sp.swap.owner.address { + return fmt.Errorf("wrong cheque parameters: expected beneficiary: %s, was: %s", sp.swap.owner.address, cheque.Beneficiary) + } + + if cheque.Timeout != 0 { + return fmt.Errorf("wrong cheque parameters: expected timeout to be 0, was: %d", cheque.Timeout) + } + + return nil +} + +func (sp *Peer) verifyChequeAgainstLast(cheque *Cheque, lastCheque *Cheque) error { + actualAmount := cheque.Amount + + if lastCheque != nil { + if cheque.Serial <= lastCheque.Serial { + return fmt.Errorf("wrong cheque parameters: expected serial larger than %d, was: %d", lastCheque.Serial, cheque.Serial) + } + + if cheque.Amount <= lastCheque.Amount { + return fmt.Errorf("wrong cheque parameters: expected amount larger than %d, was: %d", lastCheque.Amount, cheque.Amount) + } + + actualAmount -= lastCheque.Amount + } + + expectedAmount, err := sp.swap.oracle.GetPrice(cheque.Honey) + if err != nil { + return err + } + + // TODO: maybe allow some range around expectedAmount? + if expectedAmount != actualAmount { + return fmt.Errorf("unexpected exchange rate used for honey, expected %d was %d", expectedAmount, actualAmount) + } + + return nil +} + +// loadLastReceivedCheque gets the last received cheque for this peer +// cheque gets loaded from database if not already in memory +func (sp *Peer) loadLastReceivedCheque() *Cheque { + if sp.lastReceivedCheque == nil { + sp.lastReceivedCheque = sp.swap.loadLastReceivedCheque(sp.ID()) + } + return sp.lastReceivedCheque +} + +// saveLastReceivedCheque saves cheque as the last received cheque for this peer +func (sp *Peer) saveLastReceivedCheque(cheque *Cheque) error { + return sp.swap.saveLastReceivedCheque(sp.ID(), cheque) +} diff --git a/swap/swap.go b/swap/swap.go index 03381ffdb9..7157b04a05 100644 --- a/swap/swap.go +++ b/swap/swap.go @@ -323,6 +323,22 @@ func (s *Swap) loadCheque(peer enode.ID) (err error) { return } +// saveLastReceivedCheque loads the last received cheque for peer +func (s *Swap) loadLastReceivedCheque(peer enode.ID) *Cheque { + s.lock.Lock() + defer s.lock.Unlock() + var cheque *Cheque + s.stateStore.Get(peer.String()+"_cheques_received", &cheque) + return cheque +} + +// saveLastReceivedCheque saves cheque as the last received cheque for peer +func (s *Swap) saveLastReceivedCheque(peer enode.ID, cheque *Cheque) error { + s.lock.Lock() + defer s.lock.Unlock() + return s.stateStore.Put(peer.String()+"_cheques_received", cheque) +} + // Close cleans up swap func (s *Swap) Close() { s.stateStore.Close() diff --git a/swap/swap_test.go b/swap/swap_test.go index 0d324b23b4..f16c52e5d3 100644 --- a/swap/swap_test.go +++ b/swap/swap_test.go @@ -682,3 +682,47 @@ func testDeploy(ctx context.Context, backend cswap.Backend, swap *Swap) (err err } return nil } + +func testSaveAndLoadLastReceivedCheque(t *testing.T) { + swap, dir := newTestSwap(t) + defer os.RemoveAll(dir) + + testID := newDummyPeer().Peer.ID() + testCheque := newTestCheque() + + if err := swap.saveLastReceivedCheque(testID, testCheque); err != nil { + t.Fatalf("Error while saving: %s", err.Error()) + } + + returnedCheque := swap.loadLastReceivedCheque(testID) + + if returnedCheque == nil { + t.Fatalf("Could not find saved cheque") + } + + if returnedCheque.Amount != testCheque.Amount || returnedCheque.Beneficiary != testCheque.Beneficiary { + t.Fatalf("Returned cheque was different") + } +} + +func testPeerSaveAndLoadLastReceivedCheque(t *testing.T) { + swap, dir := newTestSwap(t) + defer os.RemoveAll(dir) + + testCheque := newTestCheque() + peer := NewPeer(newDummyPeer().Peer, swap, nil, common.Address{}, common.Address{}) + + if err := peer.saveLastReceivedCheque(testCheque); err != nil { + t.Fatalf("Error while saving: %s", err.Error()) + } + + returnedCheque := peer.loadLastReceivedCheque() + + if returnedCheque == nil { + t.Fatalf("Could not find saved cheque") + } + + if returnedCheque.Amount != testCheque.Amount || returnedCheque.Beneficiary != testCheque.Beneficiary { + t.Fatalf("Returned cheque was different") + } +} From 16b06602a0ca9f9eed7dda7e0e206bb39d86496b Mon Sep 17 00:00:00 2001 From: Ralph Pichler Date: Wed, 24 Jul 2019 19:14:02 +0200 Subject: [PATCH 2/4] swap: add tests for cheque verification --- swap/peer.go | 58 ++++++----- swap/swap.go | 11 ++- swap/swap_test.go | 245 ++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 268 insertions(+), 46 deletions(-) diff --git a/swap/peer.go b/swap/peer.go index 2018dcb1bb..a95276b2e2 100644 --- a/swap/peer.go +++ b/swap/peer.go @@ -77,20 +77,7 @@ func (sp *Peer) handleEmitChequeMsg(ctx context.Context, msg *EmitChequeMsg) err log.Info("received emit cheque message") cheque := msg.Cheque - if err := sp.verifyChequeProperties(msg.Cheque); err != nil { - return err - } - - lastCheque := sp.loadLastReceivedCheque() - - if err := sp.verifyChequeAgainstLast(msg.Cheque, lastCheque); err != nil { - return err - } - - if err := sp.saveLastReceivedCheque(cheque); err != nil { - log.Error("error while saving last received cheque", "peer", sp.ID().String(), "err", err.Error()) - // TODO: what do we do here? - } + sp.processAndVerifyCheque(cheque) // reset balance by amount // as this is done by the creditor, receiving the cheque, the amount should be negative, @@ -134,9 +121,38 @@ func (sp *Peer) handleErrorMsg(ctx context.Context, msg interface{}) error { return nil } +// processAndVerifyCheque verifies the cheque and compares it with the last received cheque +// if the cheque is valid it will also be saved as the new last cheque +func (sp *Peer) processAndVerifyCheque(cheque *Cheque) error { + if err := sp.verifyChequeProperties(cheque); err != nil { + return err + } + + lastCheque := sp.loadLastReceivedCheque() + + // TODO: there should probably be a lock here? + expectedAmount, err := sp.swap.oracle.GetPrice(cheque.Honey) + if err != nil { + return err + } + + if err := sp.verifyChequeAgainstLast(cheque, lastCheque, expectedAmount); err != nil { + return err + } + + if err := sp.saveLastReceivedCheque(cheque); err != nil { + log.Error("error while saving last received cheque", "peer", sp.ID().String(), "err", err.Error()) + // TODO: what do we do here? + } + + return nil +} + +// verifyChequeProperties verifies the signautre and if the cheque fields are appropiate for this peer +// it does not verify anything that requires knowing the previous cheque func (sp *Peer) verifyChequeProperties(cheque *Cheque) error { if cheque.Contract != sp.contractAddress { - return fmt.Errorf("wrong cheque parameters: expected contract: %s, was: %s", sp.contractAddress, cheque.Contract) + return fmt.Errorf("wrong cheque parameters: expected contract: %x, was: %x", sp.contractAddress, cheque.Contract) } // the beneficiary is the owner of the counterparty swap contract @@ -146,7 +162,7 @@ func (sp *Peer) verifyChequeProperties(cheque *Cheque) error { } if cheque.Beneficiary != sp.swap.owner.address { - return fmt.Errorf("wrong cheque parameters: expected beneficiary: %s, was: %s", sp.swap.owner.address, cheque.Beneficiary) + return fmt.Errorf("wrong cheque parameters: expected beneficiary: %x, was: %x", sp.swap.owner.address, cheque.Beneficiary) } if cheque.Timeout != 0 { @@ -156,7 +172,9 @@ func (sp *Peer) verifyChequeProperties(cheque *Cheque) error { return nil } -func (sp *Peer) verifyChequeAgainstLast(cheque *Cheque, lastCheque *Cheque) error { +// verifyChequeAgainstLast verifies that serial and amount are higher than in the previous cheque +// furthermore it cheques that the increase in amount is as expected +func (sp *Peer) verifyChequeAgainstLast(cheque *Cheque, lastCheque *Cheque, expectedAmount uint64) error { actualAmount := cheque.Amount if lastCheque != nil { @@ -171,11 +189,6 @@ func (sp *Peer) verifyChequeAgainstLast(cheque *Cheque, lastCheque *Cheque) erro actualAmount -= lastCheque.Amount } - expectedAmount, err := sp.swap.oracle.GetPrice(cheque.Honey) - if err != nil { - return err - } - // TODO: maybe allow some range around expectedAmount? if expectedAmount != actualAmount { return fmt.Errorf("unexpected exchange rate used for honey, expected %d was %d", expectedAmount, actualAmount) @@ -195,5 +208,6 @@ func (sp *Peer) loadLastReceivedCheque() *Cheque { // saveLastReceivedCheque saves cheque as the last received cheque for this peer func (sp *Peer) saveLastReceivedCheque(cheque *Cheque) error { + sp.lastReceivedCheque = cheque return sp.swap.saveLastReceivedCheque(sp.ID(), cheque) } diff --git a/swap/swap.go b/swap/swap.go index 7157b04a05..7fc0e42095 100644 --- a/swap/swap.go +++ b/swap/swap.go @@ -400,9 +400,9 @@ func (s *Swap) verifyChequeSig(cheque *Cheque, expectedSigner common.Address) er return nil } -// signContent signs the cheque -func (s *Swap) signContent(cheque *Cheque) ([]byte, error) { - sig, err := crypto.Sign(s.sigHashCheque(cheque), s.owner.privateKey) +// signContent signs the cheque with supplied private key +func (s *Swap) signContentWithKey(cheque *Cheque, prv *ecdsa.PrivateKey) ([]byte, error) { + sig, err := crypto.Sign(s.sigHashCheque(cheque), prv) if err != nil { return nil, err } @@ -412,6 +412,11 @@ func (s *Swap) signContent(cheque *Cheque) ([]byte, error) { return sig, nil } +// signContent signs the cheque with the owners private key +func (s *Swap) signContent(cheque *Cheque) ([]byte, error) { + return s.signContentWithKey(cheque, s.owner.privateKey) +} + // GetParams returns contract parameters (Bin, ABI) from the contract func (s *Swap) GetParams() *swap.Params { return s.contractReference.ContractParams() diff --git a/swap/swap_test.go b/swap/swap_test.go index f16c52e5d3..f3c898492e 100644 --- a/swap/swap_test.go +++ b/swap/swap_test.go @@ -54,7 +54,8 @@ var ( ownerAddress = crypto.PubkeyToAddress(ownerKey.PublicKey) beneficiaryKey, _ = crypto.HexToECDSA("6f05b0a29723ca69b1fc65d11752cee22c200cf3d2938e670547f7ae525be112") beneficiaryAddress = crypto.PubkeyToAddress(beneficiaryKey.PublicKey) - chequeSig = common.Hex2Bytes("d985613f7d8bfcf0f96f4bb00a21111beb9a675477f47e4d9b79c89f880cf99c5ab9ef4cdec7186debc51b898fe4d062a835de61fba6db390316db13d50d23941c") + testChequeSig = common.Hex2Bytes("fd3f73c7a708bb4e42471b76dabee2a0c1b9af29efb7eadb37f206bf871b81cf0c7987ad89633be930a63eba9e793cc77896131de7d9740b49da80c23c217c621c") + testChequeContract = common.HexToAddress("0x4405415b2B8c9F9aA83E151637B8378dD3bcfEDD") ) // booking represents an accounting movement in relation to a particular node: `peer` @@ -380,15 +381,13 @@ func newDummyPeerWithSpec(spec *protocols.Spec) *dummyPeer { // creates cheque structure for testing func newTestCheque() *Cheque { - contract := common.HexToAddress("0x4405415b2B8c9F9aA83E151637B8378dD3bcfEDD") - cashInDelay := 10 - cheque := &Cheque{ ChequeParams: ChequeParams{ - Contract: contract, + Contract: testChequeContract, Serial: uint64(1), Amount: uint64(42), - Timeout: uint64(cashInDelay), + Honey: uint64(42), + Timeout: uint64(0), Beneficiary: beneficiaryAddress, }, } @@ -407,7 +406,7 @@ func TestEncodeCheque(t *testing.T) { // encode the cheque encoded := swap.encodeCheque(expectedCheque) // expected value (computed through truffle/js) - expected := common.Hex2Bytes("4405415b2b8c9f9aa83e151637b8378dd3bcfeddb8d424e9662fe0837fb1d728f1ac97cebb1085fe0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000002a000000000000000000000000000000000000000000000000000000000000000a") + expected := common.Hex2Bytes("4405415b2b8c9f9aa83e151637b8378dd3bcfeddb8d424e9662fe0837fb1d728f1ac97cebb1085fe0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000002a0000000000000000000000000000000000000000000000000000000000000000") if !bytes.Equal(encoded, expected) { t.Fatalf("Unexpected encoding of cheque. Expected encoding: %x, result is: %x", expected, encoded) @@ -425,7 +424,7 @@ func TestSigHashCheque(t *testing.T) { // compute the hash that will be signed hash := swap.sigHashCheque(expectedCheque) // expected value (computed through truffle/js) - expected := common.Hex2Bytes("e431e83bed105cb66d9aa5878cb010bc21365d2e328ce7c36671f0cbd44070ae") + expected := common.Hex2Bytes("291619739fc0008915f09989411d22a29ea62eb39d86ed094ef51d6a420a1358") if !bytes.Equal(hash, expected) { t.Fatal(fmt.Sprintf("Unexpected sigHash of cheque. Expected: %x, result is: %x", expected, hash)) @@ -448,7 +447,7 @@ func TestSignContent(t *testing.T) { // sign the cheque sig, err := swap.signContent(expectedCheque) // expected value (computed through truffle/js) - expected := chequeSig + expected := testChequeSig if err != nil { t.Fatal(fmt.Sprintf("Error in signing: %s", err)) } @@ -465,7 +464,7 @@ func TestVerifyChequeSig(t *testing.T) { defer os.RemoveAll(dir) expectedCheque := newTestCheque() - expectedCheque.Sig = chequeSig + expectedCheque.Sig = testChequeSig if err := swap.verifyChequeSig(expectedCheque, ownerAddress); err != nil { t.Fatalf("Invalid signature: %v", err) @@ -479,7 +478,7 @@ func TestVerifyChequeSigWrongSigner(t *testing.T) { defer os.RemoveAll(dir) expectedCheque := newTestCheque() - expectedCheque.Sig = chequeSig + expectedCheque.Sig = testChequeSig // We expect the signer to be beneficiaryAddress but chequeSig is the signature from the owner if err := swap.verifyChequeSig(expectedCheque, beneficiaryAddress); err == nil { @@ -487,6 +486,15 @@ func TestVerifyChequeSigWrongSigner(t *testing.T) { } } +// helper function to make a signature "invalid" +func manipulateSignature(sig []byte) []byte { + invalidSig := make([]byte, len(sig)) + copy(invalidSig, sig) + // change one byte in the signature + invalidSig[27] += 2 + return invalidSig +} + // tests if verifyChequeSig reject an invalid signature func TestVerifyChequeInvalidSignature(t *testing.T) { // setup test swap object @@ -494,11 +502,7 @@ func TestVerifyChequeInvalidSignature(t *testing.T) { defer os.RemoveAll(dir) expectedCheque := newTestCheque() - - invalidSig := chequeSig[:] - // change one byte in the signature - invalidSig[27] += 2 - expectedCheque.Sig = invalidSig + expectedCheque.Sig = manipulateSignature(testChequeSig) if err := swap.verifyChequeSig(expectedCheque, ownerAddress); err == nil { t.Fatal("Valid signature, should have been invalid") @@ -683,7 +687,8 @@ func testDeploy(ctx context.Context, backend cswap.Backend, swap *Swap) (err err return nil } -func testSaveAndLoadLastReceivedCheque(t *testing.T) { +// TestSaveAndLoadLastReceivedCheque tests if a saved last received cheque can be loaded again later using the swap functions +func TestSaveAndLoadLastReceivedCheque(t *testing.T) { swap, dir := newTestSwap(t) defer os.RemoveAll(dir) @@ -705,12 +710,23 @@ func testSaveAndLoadLastReceivedCheque(t *testing.T) { } } -func testPeerSaveAndLoadLastReceivedCheque(t *testing.T) { +// newTestSwapAndPeer is a helper function to create a swap and a peer instance that fit together +func newTestSwapAndPeer(t *testing.T) (*Swap, *Peer, string) { swap, dir := newTestSwap(t) + // owner address is the beneficary (counterparty) for the peer + // that's because we expect cheques we receive to be signed by the address we would issue cheques to + peer := NewPeer(newDummyPeer().Peer, swap, nil, ownerAddress, testChequeContract) + // we need to adjust the owner address on swap because we will issue cheques to beneficiaryAddress + swap.owner.address = beneficiaryAddress + return swap, peer, dir +} + +// TestPeerSaveAndLoadLastReceivedCheque tests if a saved last received cheque can be loaded again later using the peer functions +func TestPeerSaveAndLoadLastReceivedCheque(t *testing.T) { + _, peer, dir := newTestSwapAndPeer(t) defer os.RemoveAll(dir) testCheque := newTestCheque() - peer := NewPeer(newDummyPeer().Peer, swap, nil, common.Address{}, common.Address{}) if err := peer.saveLastReceivedCheque(testCheque); err != nil { t.Fatalf("Error while saving: %s", err.Error()) @@ -719,10 +735,197 @@ func testPeerSaveAndLoadLastReceivedCheque(t *testing.T) { returnedCheque := peer.loadLastReceivedCheque() if returnedCheque == nil { - t.Fatalf("Could not find saved cheque") + t.Fatal("Could not find saved cheque") } if returnedCheque.Amount != testCheque.Amount || returnedCheque.Beneficiary != testCheque.Beneficiary { - t.Fatalf("Returned cheque was different") + t.Fatal("Returned cheque was different") + } +} + +// TestPeerVerifyChequeProperties tests that verifyChequeProperties will accept a valid cheque +func TestPeerVerifyChequeProperties(t *testing.T) { + _, peer, dir := newTestSwapAndPeer(t) + defer os.RemoveAll(dir) + + testCheque := newTestCheque() + testCheque.Sig = testChequeSig + + if err := peer.verifyChequeProperties(testCheque); err != nil { + t.Fatalf("failed to verify cheque properties: %s", err.Error()) + } +} + +// TestPeerVerifyChequeProperties tests that verifyChequeProperties will reject invalid cheques +func TestPeerVerifyChequePropertiesInvalidCheque(t *testing.T) { + swap, peer, dir := newTestSwapAndPeer(t) + defer os.RemoveAll(dir) + + // cheque with an invalid signature + testCheque := newTestCheque() + testCheque.Sig = manipulateSignature(testChequeSig) + if err := peer.verifyChequeProperties(testCheque); err == nil { + t.Fatalf("accepted cheque with invalid signature") + } + + // cheque with wrong contract + testCheque = newTestCheque() + testCheque.Contract = beneficiaryAddress + testCheque.Sig, _ = swap.signContentWithKey(testCheque, ownerKey) + if err := peer.verifyChequeProperties(testCheque); err == nil { + t.Fatalf("accepted cheque with wrong contract") + } + + // cheque with wrong beneficiary + testCheque = newTestCheque() + testCheque.Beneficiary = ownerAddress + testCheque.Sig, _ = swap.signContentWithKey(testCheque, ownerKey) + if err := peer.verifyChequeProperties(testCheque); err == nil { + t.Fatalf("accepted cheque with wrong beneficiary") + } + + // cheque with non-zero timeout + testCheque = newTestCheque() + testCheque.Timeout = 10 + testCheque.Sig, _ = swap.signContentWithKey(testCheque, ownerKey) + if err := peer.verifyChequeProperties(testCheque); err == nil { + t.Fatalf("accepted cheque with non-zero timeout") + } +} + +// TestPeerVerifyChequeAgainstLast tests that verifyChequeAgainstLast accepts a cheque with higher serial and amount +func TestPeerVerifyChequeAgainstLast(t *testing.T) { + _, peer, dir := newTestSwapAndPeer(t) + defer os.RemoveAll(dir) + + increase := uint64(10) + oldCheque := newTestCheque() + newCheque := newTestCheque() + + newCheque.Serial = oldCheque.Serial + 1 + newCheque.Amount = oldCheque.Amount + increase + + if err := peer.verifyChequeAgainstLast(newCheque, oldCheque, increase); err != nil { + t.Fatalf("failed to verify cheque compared to old cheque: %s", err.Error()) + } +} + +// TestPeerVerifyChequeAgainstLastInvalid tests that verifyChequeAgainstLast rejects cheques with lower serial or amount or an unexpected value +func TestPeerVerifyChequeAgainstLastInvalid(t *testing.T) { + _, peer, dir := newTestSwapAndPeer(t) + defer os.RemoveAll(dir) + + increase := uint64(10) + + // cheque with higher amount but same serial + oldCheque := newTestCheque() + newCheque := newTestCheque() + newCheque.Amount = oldCheque.Amount + increase + + if err := peer.verifyChequeAgainstLast(newCheque, oldCheque, increase); err == nil { + t.Fatal("accepted a cheque with same serial") + } + + // cheque with higher serial but same amount + oldCheque = newTestCheque() + newCheque = newTestCheque() + newCheque.Serial = oldCheque.Serial + 1 + + if err := peer.verifyChequeAgainstLast(newCheque, oldCheque, increase); err == nil { + t.Fatal("accepted a cheque with same amount") + } + + // cheque with amount != increase + oldCheque = newTestCheque() + newCheque = newTestCheque() + newCheque.Serial = oldCheque.Serial + 1 + newCheque.Amount = oldCheque.Amount + increase + 5 + + if err := peer.verifyChequeAgainstLast(newCheque, oldCheque, increase); err == nil { + t.Fatal("accepted a cheque with unexpected amount") + } +} + +// TestPeerProcessAndVerifyCheque tests that processAndVerifyCheque accepts a valid cheque and also saves it +func TestPeerProcessAndVerifyCheque(t *testing.T) { + swap, peer, dir := newTestSwapAndPeer(t) + defer os.RemoveAll(dir) + + // create test cheque and process + cheque := newTestCheque() + cheque.Sig, _ = swap.signContentWithKey(cheque, ownerKey) + + if err := peer.processAndVerifyCheque(cheque); err != nil { + t.Fatalf("failed to process cheque: %s", err) + } + + // verify that it was indeed saved + if peer.loadLastReceivedCheque().Serial != cheque.Serial { + t.Fatalf("last received cheque has wrong serial, was: %d, expected: %d", peer.lastReceivedCheque.Serial, cheque.Serial) + } + + // create another cheque with higher serial and amount + otherCheque := newTestCheque() + otherCheque.Serial = cheque.Serial + 1 + otherCheque.Amount = cheque.Amount + 10 + otherCheque.Honey = 10 + otherCheque.Sig, _ = swap.signContentWithKey(otherCheque, ownerKey) + + if err := peer.processAndVerifyCheque(otherCheque); err != nil { + t.Fatalf("failed to process cheque: %s", err) + } + + // verify that it was indeed saved + if peer.loadLastReceivedCheque().Serial != otherCheque.Serial { + t.Fatalf("last received cheque has wrong serial, was: %d, expected: %d", peer.lastReceivedCheque.Serial, otherCheque.Serial) + } +} + +// TestPeerProcessAndVerifyChequeInvalid verifies that processAndVerifyCheque does not accept cheques incompatible with the last cheque +// it first processes a valid cheque +// then rejects one with lower serial +// then rejects one with lower amount +func TestPeerProcessAndVerifyChequeInvalid(t *testing.T) { + swap, peer, dir := newTestSwapAndPeer(t) + defer os.RemoveAll(dir) + + // valid cheque with serial 5 + cheque := newTestCheque() + cheque.Serial = 5 + cheque.Sig, _ = swap.signContentWithKey(cheque, ownerKey) + + if err := peer.processAndVerifyCheque(cheque); err != nil { + t.Fatalf("failed to process cheque: %s", err) + } + + if peer.loadLastReceivedCheque().Serial != cheque.Serial { + t.Fatalf("last received cheque has wrong serial, was: %d, expected: %d", peer.lastReceivedCheque.Serial, cheque.Serial) + } + + // invalid cheque because serial is lower + otherCheque := newTestCheque() + otherCheque.Serial = cheque.Serial - 1 + otherCheque.Amount = cheque.Amount + 10 + otherCheque.Honey = 10 + otherCheque.Sig, _ = swap.signContentWithKey(otherCheque, ownerKey) + + if err := peer.processAndVerifyCheque(otherCheque); err == nil { + t.Fatal("accepted a cheque with lower serial") + } + + // invalid cheque because amount is lower + otherCheque = newTestCheque() + otherCheque.Serial = cheque.Serial + 1 + otherCheque.Amount = cheque.Amount - 10 + otherCheque.Honey = 10 + otherCheque.Sig, _ = swap.signContentWithKey(otherCheque, ownerKey) + + if err := peer.processAndVerifyCheque(otherCheque); err == nil { + t.Fatal("accepted a cheque with lower amount") + } + + // check that no invalid cheque was saved + if peer.loadLastReceivedCheque().Serial != cheque.Serial { + t.Fatalf("last received cheque has wrong serial, was: %d, expected: %d", peer.lastReceivedCheque.Serial, cheque.Serial) } } From a6053c6e273e0dcce3214d85e3d90fa7f1ce2a91 Mon Sep 17 00:00:00 2001 From: Ralph Pichler Date: Thu, 25 Jul 2019 12:40:47 +0200 Subject: [PATCH 3/4] address pr comments --- swap/peer.go | 6 ++++-- swap/swap.go | 3 +++ swap/swap_test.go | 14 ++++++++++++-- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/swap/peer.go b/swap/peer.go index a95276b2e2..f7b07c809c 100644 --- a/swap/peer.go +++ b/swap/peer.go @@ -77,7 +77,10 @@ func (sp *Peer) handleEmitChequeMsg(ctx context.Context, msg *EmitChequeMsg) err log.Info("received emit cheque message") cheque := msg.Cheque - sp.processAndVerifyCheque(cheque) + if err := sp.processAndVerifyCheque(cheque); err != nil { + log.Error("error invalid cheque", "from", sp.ID().String(), "err", err.Error()) + return err + } // reset balance by amount // as this is done by the creditor, receiving the cheque, the amount should be negative, @@ -157,7 +160,6 @@ func (sp *Peer) verifyChequeProperties(cheque *Cheque) error { // the beneficiary is the owner of the counterparty swap contract if err := sp.swap.verifyChequeSig(cheque, sp.beneficiary); err != nil { - log.Error("error invalid cheque", "from", sp.ID().String(), "err", err.Error()) return err } diff --git a/swap/swap.go b/swap/swap.go index 7fc0e42095..888fb18245 100644 --- a/swap/swap.go +++ b/swap/swap.go @@ -383,6 +383,9 @@ func (s *Swap) sigHashCheque(cheque *Cheque) []byte { func (s *Swap) verifyChequeSig(cheque *Cheque, expectedSigner common.Address) error { sigHash := s.sigHashCheque(cheque) + if cheque.Sig == nil { + return fmt.Errorf("tried to verify signature on cheque with sig nil") + } // copy signature to avoid modifying the original sig := make([]byte, len(cheque.Sig)) copy(sig, cheque.Sig) diff --git a/swap/swap_test.go b/swap/swap_test.go index f3c898492e..9a0b9e494a 100644 --- a/swap/swap_test.go +++ b/swap/swap_test.go @@ -882,15 +882,25 @@ func TestPeerProcessAndVerifyCheque(t *testing.T) { } // TestPeerProcessAndVerifyChequeInvalid verifies that processAndVerifyCheque does not accept cheques incompatible with the last cheque -// it first processes a valid cheque +// it first tries to process an invalid cheque +// then it processes a valid cheque // then rejects one with lower serial // then rejects one with lower amount func TestPeerProcessAndVerifyChequeInvalid(t *testing.T) { swap, peer, dir := newTestSwapAndPeer(t) defer os.RemoveAll(dir) - // valid cheque with serial 5 + // invalid cheque because wrong recipient cheque := newTestCheque() + cheque.Beneficiary = ownerAddress + cheque.Sig, _ = swap.signContentWithKey(cheque, ownerKey) + + if err := peer.processAndVerifyCheque(cheque); err == nil { + t.Fatal("accecpted an invalid cheque as first cheque") + } + + // valid cheque with serial 5 + cheque = newTestCheque() cheque.Serial = 5 cheque.Sig, _ = swap.signContentWithKey(cheque, ownerKey) From 998d8ac060c4a7ac9f6d20c37b8a5ceee08932d0 Mon Sep 17 00:00:00 2001 From: Ralph Pichler Date: Thu, 25 Jul 2019 17:14:11 +0200 Subject: [PATCH 4/4] address PR comments --- swap/peer.go | 2 +- swap/swap.go | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/swap/peer.go b/swap/peer.go index f7b07c809c..5b973bb757 100644 --- a/swap/peer.go +++ b/swap/peer.go @@ -193,7 +193,7 @@ func (sp *Peer) verifyChequeAgainstLast(cheque *Cheque, lastCheque *Cheque, expe // TODO: maybe allow some range around expectedAmount? if expectedAmount != actualAmount { - return fmt.Errorf("unexpected exchange rate used for honey, expected %d was %d", expectedAmount, actualAmount) + return fmt.Errorf("unexpected amount for honey, expected %d was %d", expectedAmount, actualAmount) } return nil diff --git a/swap/swap.go b/swap/swap.go index 888fb18245..7e925374de 100644 --- a/swap/swap.go +++ b/swap/swap.go @@ -324,12 +324,11 @@ func (s *Swap) loadCheque(peer enode.ID) (err error) { } // saveLastReceivedCheque loads the last received cheque for peer -func (s *Swap) loadLastReceivedCheque(peer enode.ID) *Cheque { +func (s *Swap) loadLastReceivedCheque(peer enode.ID) (cheque *Cheque) { s.lock.Lock() defer s.lock.Unlock() - var cheque *Cheque s.stateStore.Get(peer.String()+"_cheques_received", &cheque) - return cheque + return } // saveLastReceivedCheque saves cheque as the last received cheque for peer