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 about 3 hours ago
In general, to fix this problem we need to ensure that any user-controlled data that can reach log messages is sanitized before being logged. For plain-text logs, the main risk is log forging via control characters such as \n and \r. The minimal, behavior-preserving fix is to wrap the error returned from signAndPostMessage (and propagated through SendBundle) so that any newline and carriage-return characters in its message are removed before it is logged.
The single best fix here is to sanitize the error string at the log site in Send, where CodeQL identified the vulnerable sink:
if d.bundles {
if err := d.SendBundle(ctx, tx.FromAddress, params); err != nil {
d.lggr.Error("error sending bundle: ", err)
}
}We can change this to convert err to a string, strip \n and \r, and then log that sanitized string. This keeps the log semantics (error message text) nearly identical while preventing embedded newlines. We will:
- Add a small helper function in the same file to sanitize strings for logging by removing
\nand\r(and we can rely on the existingstringsimport). - Use this helper on
err.Error()at the log call site, replacingerrwith the sanitized message string.
All changes will be confined to pkg/txm/clientwrappers/dualbroadcast/flashbots_client.go. No new imports are required.
| @@ -27,6 +27,15 @@ | ||
|
|
||
| const flashbotsRPCTimeout = 10 * time.Second | ||
|
|
||
| // sanitizeLogMessage removes newline and carriage return characters from a log message | ||
| // to reduce the risk of log forging when logging data that may include user input. | ||
| func sanitizeLogMessage(msg string) string { | ||
| // Remove '\n' and '\r' characters which could be used to forge log entries. | ||
| msg = strings.ReplaceAll(msg, "\n", "") | ||
| msg = strings.ReplaceAll(msg, "\r", "") | ||
| return msg | ||
| } | ||
|
|
||
| type FlashbotsTxStore interface { | ||
| FetchUnconfirmedTransactions(context.Context, common.Address) ([]*types.Transaction, error) | ||
| } | ||
| @@ -107,7 +116,7 @@ | ||
| // 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) | ||
| d.lggr.Error("error sending bundle: ", sanitizeLogMessage(err.Error())) | ||
| } | ||
| } | ||
| return nil |
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)?
There was a problem hiding this comment.
Added a link to the docs.
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.