From 6b4b0970bce33516e563e09112f06fe06e0cc0ad Mon Sep 17 00:00:00 2001 From: Ralph Pichler Date: Thu, 25 Jul 2019 17:27:02 +0200 Subject: [PATCH] swap: load and save received cheques, verify amount and serial are higher (#1590) * swap: compute actual payment amount and verify honey amount and serial * swap: add tests for cheque verification * address pr comments * address PR comments --- swap/peer.go | 116 +++++++++++++++---- swap/swap.go | 29 ++++- swap/swap_test.go | 289 +++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 395 insertions(+), 39 deletions(-) diff --git a/swap/peer.go b/swap/peer.go index 31de9c3d67..5b973bb757 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,26 +77,11 @@ 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 { + if err := sp.processAndVerifyCheque(cheque); 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) - } - - // TODO: check serial and balance are higher - // reset balance by amount // as this is done by the creditor, receiving the cheque, the amount should be negative, // so that updateBalance will calculate balance + amount which result in reducing the peer's balance @@ -137,3 +123,93 @@ func (sp *Peer) handleErrorMsg(ctx context.Context, msg interface{}) error { // maybe balance disagreement 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: %x, was: %x", sp.contractAddress, cheque.Contract) + } + + // the beneficiary is the owner of the counterparty swap contract + if err := sp.swap.verifyChequeSig(cheque, sp.beneficiary); err != nil { + return err + } + + if cheque.Beneficiary != sp.swap.owner.address { + return fmt.Errorf("wrong cheque parameters: expected beneficiary: %x, was: %x", 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 +} + +// 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 { + 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 + } + + // TODO: maybe allow some range around expectedAmount? + if expectedAmount != actualAmount { + return fmt.Errorf("unexpected amount 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 { + sp.lastReceivedCheque = cheque + return sp.swap.saveLastReceivedCheque(sp.ID(), cheque) +} diff --git a/swap/swap.go b/swap/swap.go index 2f6c2cc709..8b49385d7f 100644 --- a/swap/swap.go +++ b/swap/swap.go @@ -374,6 +374,21 @@ 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 *Cheque) { + s.lock.Lock() + defer s.lock.Unlock() + s.stateStore.Get(peer.String()+"_cheques_received", &cheque) + return +} + +// 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() @@ -418,6 +433,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) @@ -435,9 +453,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 } @@ -447,6 +465,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 7e10ef5320..cf6595f0b4 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` @@ -383,15 +384,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, }, } @@ -410,7 +409,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) @@ -428,7 +427,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)) @@ -451,7 +450,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)) } @@ -468,7 +467,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) @@ -482,7 +481,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 { @@ -490,6 +489,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 @@ -497,11 +505,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") @@ -685,3 +689,256 @@ func testDeploy(ctx context.Context, backend cswap.Backend, swap *Swap) (err err } return nil } + +// 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) + + 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") + } +} + +// 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() + + if err := peer.saveLastReceivedCheque(testCheque); err != nil { + t.Fatalf("Error while saving: %s", err.Error()) + } + + returnedCheque := peer.loadLastReceivedCheque() + + if returnedCheque == nil { + t.Fatal("Could not find saved cheque") + } + + if returnedCheque.Amount != testCheque.Amount || returnedCheque.Beneficiary != testCheque.Beneficiary { + 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 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) + + // 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) + + 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) + } +}