Conversation
|
dimriou
force-pushed
the
oev-771_flashbots_batch
branch
from
7f1a37b to
6fe2367
Compare
dimriou
force-pushed
the
oev-771_flashbots_batch
branch
from
60f915c to
a1d5fa8
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Don't act on a bundle error - this is a fire and forget operation but we do want to log the error. | ||
| if d.bundles { | ||
| if err := d.SendBundle(ctx, tx.FromAddress, params); err != nil { | ||
| d.lggr.Error("error sending bundle: ", err) |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, the fix is to ensure any user-controlled data that may reach logs is encoded or sanitized before being embedded in messages. For plain text logs, at minimum, newline (\n) and carriage return (\r) characters must be removed (or escaped) so an attacker cannot break the log line structure. In this file, the tainted data originates from postReq.URL.String() and the JSON body used to build reqDesc, which is later included in an error and then logged. The safest approach is to sanitize these components when constructing reqDesc, so any derived errors are safe to log anywhere.
The best fix with minimal functional impact is: in signAndPostMessage, before building reqDesc, derive sanitized strings from postReq.URL.String() and body by removing \n and \r (using strings.ReplaceAll, which is already imported). Then construct reqDesc using these sanitized values. This ensures that any errors containing reqDesc cannot inject raw newlines into logs, while preserving the informational content of URL and body. No change is needed at the logging call on line 110; sanitizing earlier tainted components is sufficient. All changes occur in pkg/txm/clientwrappers/dualbroadcast/flashbots_client.go within the shown signAndPostMessage function, and we do not need new imports.
| @@ -135,7 +135,12 @@ | ||
| postReq.Header.Add("X-Flashbots-Origin", "chainlink") | ||
| postReq.Header.Add("Content-Type", "application/json") | ||
|
|
||
| reqDesc := fmt.Sprintf("%s %s body: %s", postReq.Method, postReq.URL.String(), string(body)) | ||
| // Sanitize URL and body to avoid log injection via newline or carriage return characters. | ||
| safeURL := strings.ReplaceAll(postReq.URL.String(), "\n", "") | ||
| safeURL = strings.ReplaceAll(safeURL, "\r", "") | ||
| safeBody := strings.ReplaceAll(string(body), "\n", "") | ||
| safeBody = strings.ReplaceAll(safeBody, "\r", "") | ||
| reqDesc := fmt.Sprintf("%s %s body: %s", postReq.Method, safeURL, safeBody) | ||
| resp, err := http.DefaultClient.Do(postReq) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("request %s failed: %w", reqDesc, err) |
pszal
left a comment
pszal
left a comment
There was a problem hiding this comment.
Nice! Some feedback below. Please consider adding more tests.
| if err != nil { | ||
| return fmt.Errorf("failed to get current block height: %w", err) | ||
| } | ||
| targetBlock := currentBlock.NumberU64() + 24 |
There was a problem hiding this comment.
would maxBlock be a more accurate var name?
|
|
||
| bodyItems = append(bodyItems, map[string]any{ | ||
| "tx": hexutil.Encode(txData), | ||
| "revertMode": "allow", // we always want to allow reverts so bundles are valid even if a single transaction within the bundle goes through |
There was a problem hiding this comment.
I don't see revertMode, only canRevert:
https://docs.flashbots.net/flashbots-mev-share/searchers/understanding-bundles#bundle-definition
There was a problem hiding this comment.
Indeed, but I followed the spec they gave me in our conversation.
There was a problem hiding this comment.
Does it make sense to document this spec somewhere (e.g., doc.go)?
2 participants
This PR enables Flashbots bundles. Every time a transaction is broadcasted, it will fetch all the past in-flight transactions and create a bundle. The bundle will use the first attempt of each transaction. The bundle broadcasting is a fire and forget process that helps nonce unblocking, so it is not tracked.