Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(rfq-relayer): gas estimation for zaps #3413

Merged
merged 21 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions services/rfq/api/model/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ type QuoteData struct {
ZapData string `json:"zap_data"`
ZapNative string `json:"zap_native"`
OriginAmountExact string `json:"origin_amount_exact"`
OriginSender string `json:"origin_sender"`
DestRecipient string `json:"dest_recipient"`
DestAmount *string `json:"dest_amount"`
RelayerAddress *string `json:"relayer_address"`
QuoteID *string `json:"quote_id"`
Expand Down
82 changes: 79 additions & 3 deletions services/rfq/relayer/pricer/fee_pricer.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import (
"context"
"errors"
"fmt"
"math/big"
"strings"
Expand Down Expand Up @@ -141,7 +142,7 @@

// Calculate the static L2 fee if it won't be incorporated by directly estimating the relay() call
// in addZapFees().
if quoteRequest == nil || quoteRequest.Transaction.ZapData == nil || len(quoteRequest.Transaction.ZapData) == 0 {
if quoteRequest == nil || quoteRequest.Transaction.ZapNative == nil || quoteRequest.Transaction.ZapData == nil {
gasEstimate, err := f.config.GetDestGasEstimate(int(destination))
if err != nil {
return nil, fmt.Errorf("could not get dest gas estimate: %w", err)
Expand Down Expand Up @@ -183,7 +184,7 @@
func (f *feePricer) addZapFees(ctx context.Context, destination uint32, denomToken string, quoteRequest *reldb.QuoteRequest, fee *big.Int) (*big.Int, error) {
span := trace.SpanFromContext(ctx)

if len(quoteRequest.Transaction.ZapData) > 0 {
if quoteRequest.Transaction.ZapData != nil {
gasEstimate, err := f.getZapGasEstimate(ctx, destination, quoteRequest)
if err != nil {
return nil, err
Expand Down Expand Up @@ -232,7 +233,12 @@
fastBridgeV2ABI = &parsedABI
}

encodedData, err := fastBridgeV2ABI.Pack(methodName, quoteRequest.RawRequest, f.relayerAddress)
rawRequest, err := encodeQuoteRequest(quoteRequest)
if err != nil {
return 0, fmt.Errorf("could not encode quote data: %w", err)
}

encodedData, err := fastBridgeV2ABI.Pack(methodName, rawRequest, f.relayerAddress)
if err != nil {
return 0, fmt.Errorf("could not encode function call: %w", err)
}
Expand All @@ -256,6 +262,76 @@
return gasEstimate, nil
}

func encodeQuoteRequest(quoteRequest *reldb.QuoteRequest) ([]byte, error) {
if quoteRequest == nil {
return nil, errors.New("quote request is nil")
}

// Create ABI types with error handling
uint16Type, err := abi.NewType("uint16", "", nil)
if err != nil {
return nil, fmt.Errorf("failed to create uint16 type: %w", err)
}
uint32Type, err := abi.NewType("uint32", "", nil)
if err != nil {
return nil, fmt.Errorf("failed to create uint32 type: %w", err)
}
addressType, err := abi.NewType("address", "", nil)
if err != nil {
return nil, fmt.Errorf("failed to create address type: %w", err)
}
uint256Type, err := abi.NewType("uint256", "", nil)
if err != nil {
return nil, fmt.Errorf("failed to create uint256 type: %w", err)
}
bytesType, err := abi.NewType("bytes", "", nil)
if err != nil {
return nil, fmt.Errorf("failed to create bytes type: %w", err)
}

// Define the ABI arguments
args := abi.Arguments{
{Type: uint16Type}, // version
{Type: uint32Type}, // originChainID
{Type: uint32Type}, // destChainID
{Type: addressType}, // originSender
{Type: addressType}, // destRecipient
{Type: addressType}, // originToken
{Type: addressType}, // destToken
{Type: uint256Type}, // originAmount
{Type: uint256Type}, // destAmount
{Type: uint256Type}, // originFeeAmount
{Type: uint256Type}, // deadline
{Type: uint256Type}, // nonce
{Type: addressType}, // exclusivityRelayer
{Type: uint256Type}, // exclusivityEndTime
{Type: uint256Type}, // zapNative
{Type: bytesType}, // zapData
}

tx := quoteRequest.Transaction

// Pack the arguments
return args.Pack(

Check failure on line 315 in services/rfq/relayer/pricer/fee_pricer.go

View workflow job for this annotation

GitHub Actions / Lint (services/rfq)

error returned from external package is unwrapped: sig: func (github.com/ethereum/go-ethereum/accounts/abi.Arguments).Pack(args ...interface{}) ([]byte, error) (wrapcheck)
uint16(2), // VERSION
tx.OriginChainId,
tx.DestChainId,
tx.OriginSender,
tx.DestRecipient,
tx.OriginToken,
tx.DestToken,
tx.OriginAmount,
tx.DestAmount,
tx.OriginFeeAmount,
tx.Deadline,
tx.Nonce,
tx.ExclusivityRelayer,
tx.ExclusivityEndTime,
tx.ZapNative,
tx.ZapData,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Wrap error returned from external package

The error returned from args.Pack() should be wrapped with additional context.

Apply this change:

-	return args.Pack(
+	packedData, err := args.Pack(
 		uint16(2), // VERSION
 		tx.OriginChainId,
 		tx.DestChainId,
 		tx.OriginSender,
 		tx.DestRecipient,
 		tx.OriginToken,
 		tx.DestToken,
 		tx.OriginAmount,
 		tx.DestAmount,
 		tx.OriginFeeAmount,
 		tx.Deadline,
 		tx.Nonce,
 		tx.ExclusivityRelayer,
 		tx.ExclusivityEndTime,
 		tx.ZapNative,
 		tx.ZapData,
 	)
+	if err != nil {
+		return nil, fmt.Errorf("failed to pack quote request: %w", err)
+	}
+	return packedData, nil
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return args.Pack(
uint16(2), // VERSION
tx.OriginChainId,
tx.DestChainId,
tx.OriginSender,
tx.DestRecipient,
tx.OriginToken,
tx.DestToken,
tx.OriginAmount,
tx.DestAmount,
tx.OriginFeeAmount,
tx.Deadline,
tx.Nonce,
tx.ExclusivityRelayer,
tx.ExclusivityEndTime,
tx.ZapNative,
tx.ZapData,
)
packedData, err := args.Pack(
uint16(2), // VERSION
tx.OriginChainId,
tx.DestChainId,
tx.OriginSender,
tx.DestRecipient,
tx.OriginToken,
tx.DestToken,
tx.OriginAmount,
tx.DestAmount,
tx.OriginFeeAmount,
tx.Deadline,
tx.Nonce,
tx.ExclusivityRelayer,
tx.ExclusivityEndTime,
tx.ZapNative,
tx.ZapData,
)
if err != nil {
return nil, fmt.Errorf("failed to pack quote request: %w", err)
}
return packedData, nil
🧰 Tools
🪛 golangci-lint (1.62.2)

315-315: error returned from external package is unwrapped: sig: func (github.com/ethereum/go-ethereum/accounts/abi.Arguments).Pack(args ...interface{}) ([]byte, error)

(wrapcheck)

🪛 GitHub Check: Lint (services/rfq)

[failure] 315-315:
error returned from external package is unwrapped: sig: func (github.com/ethereum/go-ethereum/accounts/abi.Arguments).Pack(args ...interface{}) ([]byte, error) (wrapcheck)

}

func (f *feePricer) GetTotalFee(parentCtx context.Context, origin, destination uint32, denomToken string, isQuote bool, quoteRequest *reldb.QuoteRequest) (_ *big.Int, err error) {
ctx, span := f.handler.Tracer().Start(parentCtx, "getTotalFee", trace.WithAttributes(
attribute.Int(metrics.Origin, int(origin)),
Expand Down
56 changes: 47 additions & 9 deletions services/rfq/relayer/quoter/quoter.go
Original file line number Diff line number Diff line change
Expand Up @@ -381,16 +381,11 @@
OriginAmountExact: originAmountExact,
}
if rfqRequest.Data.ZapNative != "" || rfqRequest.Data.ZapData != "" {
zapNative, ok := new(big.Int).SetString(rfqRequest.Data.ZapNative, 10)
if !ok {
return nil, fmt.Errorf("invalid zap native amount: %s", rfqRequest.Data.ZapNative)
}
quoteInput.QuoteRequest = &reldb.QuoteRequest{
Transaction: fastbridgev2.IFastBridgeV2BridgeTransactionV2{
ZapNative: zapNative,
ZapData: []byte(rfqRequest.Data.ZapData),
},
quoteRequest, err := quoteDataToQuoteRequestV2(&rfqRequest.Data)
if err != nil {
return nil, fmt.Errorf("error converting quote data to quote request: %w", err)
}
quoteInput.QuoteRequest = quoteRequest
}

rawQuote, err := m.generateQuote(ctx, quoteInput)
Expand Down Expand Up @@ -432,6 +427,49 @@
return resp, nil
}

func quoteDataToQuoteRequestV2(quoteData *model.QuoteData) (*reldb.QuoteRequest, error) {
if quoteData == nil {
return nil, errors.New("quote data is nil")
}

originAmount, ok := new(big.Int).SetString(quoteData.OriginAmountExact, 10)
if !ok {
return nil, errors.New("invalid origin amount")
}
destAmount := originAmount // assume dest amount same as origin amount for estimation purposes
originFeeAmount := big.NewInt(0)
nonce := big.NewInt(0)
exclusivityEndTime := big.NewInt(0)
zapNative, ok := new(big.Int).SetString(quoteData.ZapNative, 10)
if !ok {
return nil, errors.New("invalid zap native")
}
deadline := new(big.Int).Sub(new(big.Int).Exp(big.NewInt(2), big.NewInt(256), nil), big.NewInt(1))
exclusivityRelayer := common.HexToAddress("")

quoteRequest := &reldb.QuoteRequest{
Transaction: fastbridgev2.IFastBridgeV2BridgeTransactionV2{
OriginChainId: uint32(quoteData.OriginChainID),

Check failure on line 452 in services/rfq/relayer/quoter/quoter.go

View workflow job for this annotation

GitHub Actions / Lint (services/rfq)

G115: integer overflow conversion int -> uint32 (gosec)
DestChainId: uint32(quoteData.DestChainID),

Check failure on line 453 in services/rfq/relayer/quoter/quoter.go

View workflow job for this annotation

GitHub Actions / Lint (services/rfq)

G115: integer overflow conversion int -> uint32 (gosec)
Comment on lines +453 to +454
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential integer overflow when converting int to uint32

Casting quoteData.OriginChainID and quoteData.DestChainID from int to uint32 without validation may cause integer overflows if the values are negative or exceed the uint32 maximum. This can lead to unexpected behavior or security vulnerabilities. Validate the values before casting to ensure they are within the valid uint32 range.

Apply this diff to add validation:

+    if quoteData.OriginChainID < 0 || quoteData.OriginChainID > math.MaxUint32 {
+        return nil, fmt.Errorf("OriginChainID out of range: %d", quoteData.OriginChainID)
+    }
+    if quoteData.DestChainID < 0 || quoteData.DestChainID > math.MaxUint32 {
+        return nil, fmt.Errorf("DestChainID out of range: %d", quoteData.DestChainID)
+    }
     quoteRequest := &reldb.QuoteRequest{
         Transaction: fastbridgev2.IFastBridgeV2BridgeTransactionV2{
             OriginChainId:      uint32(quoteData.OriginChainID),
             DestChainId:        uint32(quoteData.DestChainID),

Alternatively, if OriginChainID and DestChainID are guaranteed to be non-negative and within uint32 limits, consider changing their types to uint32 in the QuoteData struct to prevent unnecessary casting and potential errors.

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 GitHub Check: Lint (services/rfq)

[failure] 452-452:
G115: integer overflow conversion int -> uint32 (gosec)


[failure] 453-453:
G115: integer overflow conversion int -> uint32 (gosec)

OriginSender: common.HexToAddress(quoteData.OriginSender),
DestRecipient: common.HexToAddress(quoteData.DestRecipient),
Comment on lines +455 to +456
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Validate sender and recipient addresses.

The function should validate that the sender and recipient addresses are not zero addresses to prevent potential issues with transactions.

+	if quoteData.OriginSender == "" || common.HexToAddress(quoteData.OriginSender) == (common.Address{}) {
+		return nil, errors.New("invalid origin sender address")
+	}
+	if quoteData.DestRecipient == "" || common.HexToAddress(quoteData.DestRecipient) == (common.Address{}) {
+		return nil, errors.New("invalid destination recipient address")
+	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
OriginSender: common.HexToAddress(quoteData.OriginSender),
DestRecipient: common.HexToAddress(quoteData.DestRecipient),
if quoteData.OriginSender == "" || common.HexToAddress(quoteData.OriginSender) == (common.Address{}) {
return nil, errors.New("invalid origin sender address")
}
if quoteData.DestRecipient == "" || common.HexToAddress(quoteData.DestRecipient) == (common.Address{}) {
return nil, errors.New("invalid destination recipient address")
}
OriginSender: common.HexToAddress(quoteData.OriginSender),
DestRecipient: common.HexToAddress(quoteData.DestRecipient),

OriginToken: common.HexToAddress(quoteData.OriginTokenAddr),
DestToken: common.HexToAddress(quoteData.DestTokenAddr),
OriginAmount: originAmount,
DestAmount: destAmount,
OriginFeeAmount: originFeeAmount,
Deadline: deadline,
Nonce: nonce,
ExclusivityRelayer: exclusivityRelayer,
ExclusivityEndTime: exclusivityEndTime,
ZapNative: zapNative,
ZapData: []byte(quoteData.ZapData),
},
}

return quoteRequest, nil
}

// GetPrice gets the price of a token.
func (m *Manager) GetPrice(parentCtx context.Context, tokenName string) (_ float64, err error) {
ctx, span := m.metricsHandler.Tracer().Start(parentCtx, "GetPrice")
Expand Down
Loading