From 98b90543972d37e4c00196f3f00bcf5f380ea04d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deividas=20Kar=C5=BEinauskas?= Date: Fri, 9 Aug 2024 12:17:12 +0300 Subject: [PATCH] Write Chain Target: Validate that signed report metadata matches request metadata (#14066) * Hardcoded value for call with exact gas * Record gasProvided in route function * Add a getter for transmission gas limit * Update snapshot * Changeset * Remove unused import * Rename to gas limit * Update gethwrappers * Uncomment test code * Remove copy/pasta comment * Slight rename * Allow retrying transmissions with more gas * Only allow retrying failed transmissions * Update snapshot * Fix state for InvalidReceiver check * Check for initial state * Actually store gas limit provided to receiver * Update gethwrappers * Remove unused struct * Correctly mark invalid receiver when receiver interface unsupported * Create TransmissionInfo struct * Update gethwrappers * Bump gas limit * Bump gas even more * Update KeystoneFeedsConsumer.sol to implement IERC165 * Use getTransmissionInfo * Use TransmissionState to determine if transmission should be created * Fix test * Fix trailing line * Update a mock to the GetLatestValue("getTransmissionInfo") call in a test * Remove TODO + replace panic with err * Remove redundant empty lines * Typo * Fix nil pointer dereference in write target implementation * Remove unused constant * Name mapping values * Add changeset * Validate that report metadata matches request metadata * Derive report metadata length from the struct * Bytes() => Encode() * Simplify decoding * Undo redundant change * More simplifications * More cleanup * Remove redundant comment * Changeset * Update tests --------- Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com> --- .changeset/tall-poems-swim.md | 5 + core/capabilities/targets/write_target.go | 145 +++++++++++++----- .../capabilities/targets/write_target_test.go | 88 +++++------ core/services/relay/evm/write_target_test.go | 137 ++++++----------- 4 files changed, 198 insertions(+), 177 deletions(-) create mode 100644 .changeset/tall-poems-swim.md diff --git a/.changeset/tall-poems-swim.md b/.changeset/tall-poems-swim.md new file mode 100644 index 00000000000..0408383bd03 --- /dev/null +++ b/.changeset/tall-poems-swim.md @@ -0,0 +1,5 @@ +--- +"chainlink": patch +--- + +#internal diff --git a/core/capabilities/targets/write_target.go b/core/capabilities/targets/write_target.go index 4524c4fd449..282a4741a6a 100644 --- a/core/capabilities/targets/write_target.go +++ b/core/capabilities/targets/write_target.go @@ -1,7 +1,9 @@ package targets import ( + "bytes" "context" + "encoding/binary" "encoding/hex" "fmt" "math/big" @@ -13,7 +15,6 @@ import ( "github.com/smartcontractkit/chainlink-common/pkg/capabilities/consensus/ocr3/types" commontypes "github.com/smartcontractkit/chainlink-common/pkg/types" "github.com/smartcontractkit/chainlink-common/pkg/types/query/primitives" - "github.com/smartcontractkit/chainlink-common/pkg/values" "github.com/smartcontractkit/chainlink/v2/core/logger" ) @@ -21,9 +22,6 @@ var ( _ capabilities.ActionCapability = &WriteTarget{} ) -// required field of target's config in the workflow spec -const signedReportField = "signed_report" - type WriteTarget struct { cr commontypes.ContractReader cw commontypes.ChainWriter @@ -71,22 +69,105 @@ func NewWriteTarget(lggr logger.Logger, id string, cr commontypes.ContractReader } } -type EvmConfig struct { +// Note: This should be a shared type that the OCR3 package validates as well +type ReportV1Metadata struct { + Version uint8 + WorkflowExecutionID [32]byte + Timestamp uint32 + DonID uint32 + DonConfigVersion uint32 + WorkflowCID [32]byte + WorkflowName [10]byte + WorkflowOwner [20]byte + ReportID [2]byte +} + +func (rm ReportV1Metadata) Encode() ([]byte, error) { + buf := new(bytes.Buffer) + err := binary.Write(buf, binary.BigEndian, rm) + if err != nil { + return nil, err + } + return buf.Bytes(), nil +} + +func (rm ReportV1Metadata) Length() int { + bytes, err := rm.Encode() + if err != nil { + return 0 + } + return len(bytes) +} + +func decodeReportMetadata(data []byte) (metadata ReportV1Metadata, err error) { + if len(data) < metadata.Length() { + return metadata, fmt.Errorf("data too short: %d bytes", len(data)) + } + return metadata, binary.Read(bytes.NewReader(data[:metadata.Length()]), binary.BigEndian, &metadata) +} + +type Config struct { + // Address of the contract that will get the forwarded report Address string } -func parseConfig(rawConfig *values.Map) (config EvmConfig, err error) { - if rawConfig == nil { - return config, fmt.Errorf("missing config field") +type Inputs struct { + SignedReport types.SignedReport +} + +type Request struct { + Metadata capabilities.RequestMetadata + Config Config + Inputs Inputs +} + +func evaluate(rawRequest capabilities.CapabilityRequest) (r Request, err error) { + r.Metadata = rawRequest.Metadata + + if rawRequest.Config == nil { + return r, fmt.Errorf("missing config field") + } + + if err = rawRequest.Config.UnwrapTo(&r.Config); err != nil { + return r, err + } + + if !common.IsHexAddress(r.Config.Address) { + return r, fmt.Errorf("'%v' is not a valid address", r.Config.Address) } - if err := rawConfig.UnwrapTo(&config); err != nil { - return config, err + if rawRequest.Inputs == nil { + return r, fmt.Errorf("missing inputs field") } - if !common.IsHexAddress(config.Address) { - return config, fmt.Errorf("'%v' is not a valid address", config.Address) + + // required field of target's config in the workflow spec + const signedReportField = "signed_report" + signedReport, ok := rawRequest.Inputs.Underlying[signedReportField] + if !ok { + return r, fmt.Errorf("missing required field %s", signedReportField) + } + + if err = signedReport.UnwrapTo(&r.Inputs.SignedReport); err != nil { + return r, err + } + + reportMetadata, err := decodeReportMetadata(r.Inputs.SignedReport.Report) + if err != nil { + return r, err + } + + if reportMetadata.Version != 1 { + return r, fmt.Errorf("unsupported report version: %d", reportMetadata.Version) } - return config, nil + + if hex.EncodeToString(reportMetadata.WorkflowExecutionID[:]) != rawRequest.Metadata.WorkflowExecutionID || + hex.EncodeToString(reportMetadata.WorkflowOwner[:]) != rawRequest.Metadata.WorkflowOwner || + hex.EncodeToString(reportMetadata.WorkflowName[:]) != rawRequest.Metadata.WorkflowName || + hex.EncodeToString(reportMetadata.WorkflowCID[:]) != rawRequest.Metadata.WorkflowID { + return r, fmt.Errorf("report metadata does not match request metadata. reportMetadata: %+v, requestMetadata: %+v", reportMetadata, rawRequest.Metadata) + } + + return r, nil } func success() <-chan capabilities.CapabilityResponse { @@ -98,7 +179,7 @@ func success() <-chan capabilities.CapabilityResponse { return callback } -func (cap *WriteTarget) Execute(ctx context.Context, request capabilities.CapabilityRequest) (<-chan capabilities.CapabilityResponse, error) { +func (cap *WriteTarget) Execute(ctx context.Context, rawRequest capabilities.CapabilityRequest) (<-chan capabilities.CapabilityResponse, error) { // Bind to the contract address on the write path. // Bind() requires a connection to the node's RPCs and // cannot be run during initialization. @@ -114,47 +195,27 @@ func (cap *WriteTarget) Execute(ctx context.Context, request capabilities.Capabi cap.bound = true } - cap.lggr.Debugw("Execute", "request", request) + cap.lggr.Debugw("Execute", "rawRequest", rawRequest) - reqConfig, err := parseConfig(request.Config) + request, err := evaluate(rawRequest) if err != nil { return nil, err } - if request.Inputs == nil { - return nil, fmt.Errorf("missing inputs field") - } - - signedReport, ok := request.Inputs.Underlying[signedReportField] - if !ok { - return nil, fmt.Errorf("missing required field %s", signedReportField) - } - - inputs := types.SignedReport{} - if err = signedReport.UnwrapTo(&inputs); err != nil { - return nil, err - } - - if len(inputs.Report) == 0 { - // We received any empty report -- this means we should skip transmission. - cap.lggr.Debugw("Skipping empty report", "request", request) - return success(), nil - } - // TODO: validate encoded report is prefixed with workflowID and executionID that match the request meta - rawExecutionID, err := hex.DecodeString(request.Metadata.WorkflowExecutionID) if err != nil { return nil, err } + // Check whether value was already transmitted on chain queryInputs := struct { Receiver string WorkflowExecutionID []byte ReportId []byte }{ - Receiver: reqConfig.Address, + Receiver: request.Config.Address, WorkflowExecutionID: rawExecutionID, - ReportId: inputs.ID, + ReportId: request.Inputs.SignedReport.ID, } var transmissionInfo TransmissionInfo if err = cap.cr.GetLatestValue(ctx, "forwarder", "getTransmissionInfo", primitives.Unconfirmed, queryInputs, &transmissionInfo); err != nil { @@ -163,7 +224,7 @@ func (cap *WriteTarget) Execute(ctx context.Context, request capabilities.Capabi switch { case transmissionInfo.State == 0: // NOT_ATTEMPTED - cap.lggr.Infow("non-empty report - tranasmission not attempted - attempting to push to txmgr", "request", request, "reportLen", len(inputs.Report), "reportContextLen", len(inputs.Context), "nSignatures", len(inputs.Signatures), "executionID", request.Metadata.WorkflowExecutionID) + cap.lggr.Infow("non-empty report - tranasmission not attempted - attempting to push to txmgr", "request", request, "reportLen", len(request.Inputs.SignedReport.Report), "reportContextLen", len(request.Inputs.SignedReport.Context), "nSignatures", len(request.Inputs.SignedReport.Signatures), "executionID", request.Metadata.WorkflowExecutionID) case transmissionInfo.State == 1: // SUCCEEDED cap.lggr.Infow("returning without a tranmission attempt - report already onchain ", "executionID", request.Metadata.WorkflowExecutionID) return success(), nil @@ -175,7 +236,7 @@ func (cap *WriteTarget) Execute(ctx context.Context, request capabilities.Capabi cap.lggr.Infow("returning without a tranmission attempt - transmission already attempted and failed, sufficient gas was provided", "executionID", request.Metadata.WorkflowExecutionID, "receiverGasMinimum", cap.receiverGasMinimum, "transmissionGasLimit", transmissionInfo.GasLimit) return success(), nil } else { - cap.lggr.Infow("non-empty report - retrying a failed transmission - attempting to push to txmgr", "request", request, "reportLen", len(inputs.Report), "reportContextLen", len(inputs.Context), "nSignatures", len(inputs.Signatures), "executionID", request.Metadata.WorkflowExecutionID, "receiverGasMinimum", cap.receiverGasMinimum, "transmissionGasLimit", transmissionInfo.GasLimit) + cap.lggr.Infow("non-empty report - retrying a failed transmission - attempting to push to txmgr", "request", request, "reportLen", len(request.Inputs.SignedReport.Report), "reportContextLen", len(request.Inputs.SignedReport.Context), "nSignatures", len(request.Inputs.SignedReport.Signatures), "executionID", request.Metadata.WorkflowExecutionID, "receiverGasMinimum", cap.receiverGasMinimum, "transmissionGasLimit", transmissionInfo.GasLimit) } default: return nil, fmt.Errorf("unexpected transmission state: %v", transmissionInfo.State) @@ -194,7 +255,7 @@ func (cap *WriteTarget) Execute(ctx context.Context, request capabilities.Capabi RawReport []byte ReportContext []byte Signatures [][]byte - }{reqConfig.Address, inputs.Report, inputs.Context, inputs.Signatures} + }{request.Config.Address, request.Inputs.SignedReport.Report, request.Inputs.SignedReport.Context, request.Inputs.SignedReport.Signatures} if req.RawReport == nil { req.RawReport = make([]byte, 0) diff --git a/core/capabilities/targets/write_target_test.go b/core/capabilities/targets/write_target_test.go index 0fa750911dd..522fee32513 100644 --- a/core/capabilities/targets/write_target_test.go +++ b/core/capabilities/targets/write_target_test.go @@ -2,6 +2,7 @@ package targets_test import ( "context" + "encoding/hex" "errors" "math/big" "testing" @@ -38,14 +39,36 @@ func TestWriteTarget(t *testing.T) { }) require.NoError(t, err) + reportMetadata := targets.ReportV1Metadata{ + Version: 1, + WorkflowExecutionID: [32]byte{}, + Timestamp: 0, + DonID: 0, + DonConfigVersion: 0, + WorkflowCID: [32]byte{}, + WorkflowName: [10]byte{}, + WorkflowOwner: [20]byte{}, + ReportID: [2]byte{}, + } + + reportMetadataBytes, err := reportMetadata.Encode() + require.NoError(t, err) + validInputs, err := values.NewMap(map[string]any{ "signed_report": map[string]any{ - "report": []byte{1, 2, 3}, + "report": reportMetadataBytes, "signatures": [][]byte{}, }, }) require.NoError(t, err) + validMetadata := capabilities.RequestMetadata{ + WorkflowID: hex.EncodeToString(reportMetadata.WorkflowCID[:]), + WorkflowOwner: hex.EncodeToString(reportMetadata.WorkflowOwner[:]), + WorkflowName: hex.EncodeToString(reportMetadata.WorkflowName[:]), + WorkflowExecutionID: hex.EncodeToString(reportMetadata.WorkflowExecutionID[:]), + } + cr.On("Bind", mock.Anything, []types.BoundContract{ { Address: forwarderAddr, @@ -69,11 +92,9 @@ func TestWriteTarget(t *testing.T) { t.Run("succeeds with valid report", func(t *testing.T) { req := capabilities.CapabilityRequest{ - Metadata: capabilities.RequestMetadata{ - WorkflowID: "test-id", - }, - Config: config, - Inputs: validInputs, + Metadata: validMetadata, + Config: config, + Inputs: validInputs, } ch, err2 := writeTarget.Execute(ctx, req) @@ -82,36 +103,11 @@ func TestWriteTarget(t *testing.T) { require.NotNil(t, response) }) - t.Run("succeeds with empty report", func(t *testing.T) { - emptyInputs, err2 := values.NewMap(map[string]any{ - "signed_report": map[string]any{ - "report": []byte{}, - }, - "signatures": [][]byte{}, - }) - - require.NoError(t, err2) - req := capabilities.CapabilityRequest{ - Metadata: capabilities.RequestMetadata{ - WorkflowExecutionID: "test-id", - }, - Config: config, - Inputs: emptyInputs, - } - - ch, err2 := writeTarget.Execute(ctx, req) - require.NoError(t, err2) - response := <-ch - require.Nil(t, response.Value) - }) - t.Run("fails when ChainReader's GetLatestValue returns error", func(t *testing.T) { req := capabilities.CapabilityRequest{ - Metadata: capabilities.RequestMetadata{ - WorkflowID: "test-id", - }, - Config: config, - Inputs: validInputs, + Metadata: validMetadata, + Config: config, + Inputs: validInputs, } cr.On("GetLatestValue", mock.Anything, "forwarder", "getTransmissionInfo", mock.Anything, mock.Anything, mock.Anything).Return(errors.New("reader error")) @@ -121,11 +117,9 @@ func TestWriteTarget(t *testing.T) { t.Run("fails when ChainWriter's SubmitTransaction returns error", func(t *testing.T) { req := capabilities.CapabilityRequest{ - Metadata: capabilities.RequestMetadata{ - WorkflowID: "test-id", - }, - Config: config, - Inputs: validInputs, + Metadata: validMetadata, + Config: config, + Inputs: validInputs, } cw.On("SubmitTransaction", mock.Anything, "forwarder", "report", mock.Anything, mock.Anything, forwarderAddr, mock.Anything, mock.Anything).Return(errors.New("writer error")) @@ -152,11 +146,9 @@ func TestWriteTarget(t *testing.T) { t.Run("fails with nil config", func(t *testing.T) { req := capabilities.CapabilityRequest{ - Metadata: capabilities.RequestMetadata{ - WorkflowID: "test-id", - }, - Config: nil, - Inputs: validInputs, + Metadata: validMetadata, + Config: nil, + Inputs: validInputs, } _, err2 := writeTarget.Execute(ctx, req) require.Error(t, err2) @@ -164,11 +156,9 @@ func TestWriteTarget(t *testing.T) { t.Run("fails with nil inputs", func(t *testing.T) { req := capabilities.CapabilityRequest{ - Metadata: capabilities.RequestMetadata{ - WorkflowID: "test-id", - }, - Config: config, - Inputs: nil, + Metadata: validMetadata, + Config: config, + Inputs: nil, } _, err2 := writeTarget.Execute(ctx, req) require.Error(t, err2) diff --git a/core/services/relay/evm/write_target_test.go b/core/services/relay/evm/write_target_test.go index 57a0f80f8d3..54e36714226 100644 --- a/core/services/relay/evm/write_target_test.go +++ b/core/services/relay/evm/write_target_test.go @@ -2,6 +2,7 @@ package evm_test import ( "bytes" + "encoding/hex" "errors" "fmt" "math/big" @@ -146,14 +147,53 @@ func TestEvmWrite(t *testing.T) { }) require.NoError(t, err) + reportMetadata := targets.ReportV1Metadata{ + Version: 1, + WorkflowExecutionID: [32]byte{}, + Timestamp: 0, + DonID: 0, + DonConfigVersion: 0, + WorkflowCID: [32]byte{}, + WorkflowName: [10]byte{}, + WorkflowOwner: [20]byte{}, + ReportID: [2]byte{}, + } + + reportMetadataBytes, err := reportMetadata.Encode() + require.NoError(t, err) + + signatures := [][]byte{} + + validInputs, err := values.NewMap(map[string]any{ + "signed_report": map[string]any{ + "report": reportMetadataBytes, + "signatures": signatures, + "context": []byte{4, 5}, + "id": []byte{9, 9}, + }, + }) + require.NoError(t, err) + + validMetadata := capabilities.RequestMetadata{ + WorkflowID: hex.EncodeToString(reportMetadata.WorkflowCID[:]), + WorkflowOwner: hex.EncodeToString(reportMetadata.WorkflowOwner[:]), + WorkflowName: hex.EncodeToString(reportMetadata.WorkflowName[:]), + WorkflowExecutionID: hex.EncodeToString(reportMetadata.WorkflowExecutionID[:]), + } + + validConfig, err := values.NewMap(map[string]any{ + "Address": evmCfg.EVM().Workflow().ForwarderAddress().String(), + }) + require.NoError(t, err) + txManager.On("CreateTransaction", mock.Anything, mock.Anything).Return(txmgr.Tx{}, nil).Run(func(args mock.Arguments) { req := args.Get(1).(txmgr.TxRequest) payload := make(map[string]any) method := forwardABI.Methods["report"] err = method.Inputs.UnpackIntoMap(payload, req.EncodedPayload[4:]) require.NoError(t, err) - require.Equal(t, []byte{0x1, 0x2, 0x3}, payload["rawReport"]) - require.Equal(t, [][]byte{}, payload["signatures"]) + require.Equal(t, reportMetadataBytes, payload["rawReport"]) + require.Equal(t, signatures, payload["signatures"]) }).Once() t.Run("succeeds with valid report", func(t *testing.T) { @@ -161,59 +201,10 @@ func TestEvmWrite(t *testing.T) { capability, err := evm.NewWriteTarget(ctx, relayer, chain, lggr) require.NoError(t, err) - config, err := values.NewMap(map[string]any{ - "Address": evmCfg.EVM().Workflow().ForwarderAddress().String(), - }) - require.NoError(t, err) - - inputs, err := values.NewMap(map[string]any{ - "signed_report": map[string]any{ - "report": []byte{1, 2, 3}, - "signatures": [][]byte{}, - "context": []byte{4, 5}, - "id": []byte{9, 9}, - }, - }) - require.NoError(t, err) - - req := capabilities.CapabilityRequest{ - Metadata: capabilities.RequestMetadata{ - WorkflowID: "test-id", - }, - Config: config, - Inputs: inputs, - } - - ch, err := capability.Execute(ctx, req) - require.NoError(t, err) - - response := <-ch - require.Nil(t, response.Err) - }) - - t.Run("succeeds with empty report", func(t *testing.T) { - ctx := testutils.Context(t) - capability, err := evm.NewWriteTarget(ctx, relayer, chain, logger.TestLogger(t)) - require.NoError(t, err) - - config, err := values.NewMap(map[string]any{ - "Address": evmCfg.EVM().Workflow().ForwarderAddress().String(), - }) - require.NoError(t, err) - - inputs, err := values.NewMap(map[string]any{ - "signed_report": map[string]any{ - "report": nil, - }, - }) - require.NoError(t, err) - req := capabilities.CapabilityRequest{ - Metadata: capabilities.RequestMetadata{ - WorkflowID: "test-id", - }, - Config: config, - Inputs: inputs, + Metadata: validMetadata, + Config: validConfig, + Inputs: validInputs, } ch, err := capability.Execute(ctx, req) @@ -233,19 +224,10 @@ func TestEvmWrite(t *testing.T) { }) require.NoError(t, err) - inputs, err := values.NewMap(map[string]any{ - "signed_report": map[string]any{ - "report": nil, - }, - }) - require.NoError(t, err) - req := capabilities.CapabilityRequest{ - Metadata: capabilities.RequestMetadata{ - WorkflowID: "test-id", - }, - Config: invalidConfig, - Inputs: inputs, + Metadata: validMetadata, + Config: invalidConfig, + Inputs: validInputs, } _, err = capability.Execute(ctx, req) @@ -257,27 +239,10 @@ func TestEvmWrite(t *testing.T) { capability, err := evm.NewWriteTarget(ctx, relayer, chain, logger.TestLogger(t)) require.NoError(t, err) - config, err := values.NewMap(map[string]any{ - "Address": evmCfg.EVM().Workflow().ForwarderAddress().String(), - }) - require.NoError(t, err) - - inputs, err := values.NewMap(map[string]any{ - "signed_report": map[string]any{ - "report": []byte{1, 2, 3}, - "signatures": [][]byte{}, - "context": []byte{4, 5}, - "id": []byte{9, 9}, - }, - }) - require.NoError(t, err) - req := capabilities.CapabilityRequest{ - Metadata: capabilities.RequestMetadata{ - WorkflowID: "test-id", - }, - Config: config, - Inputs: inputs, + Metadata: validMetadata, + Config: validConfig, + Inputs: validInputs, } txManager.On("CreateTransaction", mock.Anything, mock.Anything).Return(txmgr.Tx{}, errors.New("TXM error"))