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
Overriding transfer handler #419
Overriding transfer handler #419
Conversation
…l/fix/h-5-overriding-transfer-handler
Everything looks good. Just had to fix some merge conflicts. Ready to merge now |
@@ -114,8 +131,10 @@ contract ConnextHandler { | |||
external | |||
onlyConnextRouter | |||
{ | |||
_failedTxns[transferId] = | |||
FailedTxn(transferId, amount, asset, originSender, originDomain, actions, args); | |||
if (!isTransferIdRecorded(transferId)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this solves overriding vector, but adds a different attacking vector
now anyone can pre-call xBundle with the given transferId
and make the legit recordFail
revert
we had mentioned this previously as well
https://www.notion.so/0xmacro/Leads-Questions-on-Router-14d2593767a640258c902031573da7c8?pvs=4#c508aacbca3847aca17a1372879ed074
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @abhishekvispute good point, decided to essentially record all attempted transferIds with an increasing nonce.
Please refer to #536.
This pull request addresses H-5 of Macro audit report