QA Report

[howlbot-integration[bot]]

See the markdown file with the details of this report here.

[c4-judge]

trust1995 marked the issue as grade-c

[dontonka]

Dear judge, with all due respect, I think my Low-4 should be made duplicate of #14 as I'm identifying the same root cause and also indirectly suggesting the same recommendation which is to apply the exitEarly path to this event.

Additionally, this matter was also discussed in private in Discord with the sponsor during the contest and here is the snapshot of this discussion, which is the main reason why I didn't submit this as a Medium and only as a Low. The sponsor text can be confirmed by @the-eridanus.

dontonka — 06/06/2024 8:33 AM
3) For  transferAllowanceEvent event i can read the following comment, but there is nothing in the code that address this. Was that meant to also use earlyExit path similar to transferOutEvent?

sysrqb — 06/10/2024 9:21 AM
on this, I think the comment is confusingly worded (and I should fix it up) - but the transfer allowance event is only emitted by the router code, and seeing the code it can only be emitted once for each vault transfer, so early-out or not in the log parser code doesn't make a difference

So based on those facts, I would like you to consider to make my finding as a full duplicate of #14, as while the selected finding have more content, it doesn't convey really more information for the sponsor then my 3 lines here.

[trust1995]

Dear judge, with all due respect, I think my Low-4 should be made duplicate of https://github.com/code-423n4/2024-06-thorchain-findings/issues/14 as I'm identifying the same root cause and also indirectly suggesting the same recommendation which is to apply the exitEarly path to this event.

Unfortunately I will not be able to upgrade it because the Supreme Court ruling states a baseline for H/M is stated end-impact, showing a root cause is not enough.

[dontonka]

@the-eridanus @trust1995 could you guys provide further details why 14 would warrant Medium severity from your perspective, I'm just trying to understand really what are the negative impacts of this missing early exit, such that we can all agree if the current severity make sense or not.

The impact claimed by the warden are as follow.

This breaks the expectation that only one TransferAllowance event is processed per transaction, and may result in unaccounted for or unexpected asset transfer or churning between more than one vault per transaction.

From the discussion we had in Discord with your colleague sysrqb, my interpretation was that this was not really a problem per say, as this is dictated by THORChain node which will call the router and will emit one event per transaction, and that code is working as expected, which is why that was falling into QA category from my perspective. It's like you would protect your code twice for the samething, which is redundant. Also if a bug would be introduced in the future, that would be OOS.

The fact that you labeled Sponsor confirmed doesn't seem to confirm the severity (similar to #18) , but simply that this should be fixed.

• How often vault transfer would occurs?
• What can go wrong when the product is working as expected while this missing early is missing ?

The only issue I could see is a griefing attack scenario (same as mine) as you will open this with the whitlisting, but the warden didn't added such impact at all, he doesn't bring anything new really on the table. But I definitely recommend to fix it for the same reasons exposed in 22.

[the-eridanus]

@the-eridanus @trust1995 could you guys provide further details why 14 would warrant Medium severity from your perspective, I'm just trying to understand really what are the negative impacts of this missing early exit, such that we can all agree if the current severity make sense or not.

The impact claimed by the warden are as follow.

This breaks the expectation that only one TransferAllowance event is processed per transaction, and may result in unaccounted for or unexpected asset transfer or churning between more than one vault per transaction.

From the discussion we had in Discord with your colleague sysrqb, my interpretation was that this was not really a problem per say, as this is dictated by THORChain node which will call the router and will emit one event per transaction, and that code is working as expected, which is why that was falling into QA category from my perspective. It's like you would protect your code twice for the samething, which is redundant. Also if a bug would be introduced in the future, that would be OOS.

The fact that you labeled Sponsor confirmed doesn't seem to confirm the severity (similar to #18) , but simply that this should be fixed.

• How often vault transfer would occurs?
• What can go wrong when the product is working as expected while this missing early is missing ?

The only issue I could see is a griefing attack scenario (same as mine) as you will open this with the whitlisting, but the warden didn't added such impact at all, he doesn't bring anything new really on the table. But I definitely recommend to fix it for the same reasons exposed in 22.

Reviewing that issue again, I agree it should be categorized as low severity. The impact is not high even though the code should still be changed

[hGq2Wnl]

In the interest of clarity, this is what the sponsors told me about the TransferAllowance event, which corresponds to the transferAllowanceEvent case in smartcontract_log_parser:GetTxInItem after I (GhK3Ndf ) asked a very similar question in a private thread as shown in the warden's comment above:

GhK3Ndf — 06/12/2024 7:04 PM Hi (sponsors) ,apologies for a new thread, but is the TransferAllowance event only supposed to be emited once per block/batch of transactions, like with TransferOut?

Referring to this comment: https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/bifrost/pkg/chainclients/shared/evm/smartcontract_log_parser.go#L245

Thanks!

GhK3Ndf — 06/12/2024 8:13 PM (I tag additional sponsor )

olegpetrov — 06/12/2024 8:13 PM Eridanus know more about this than me!

Eridanus (9R) — 06/12/2024 8:15 PM Correct, there should be only one TransferAllowance event per tx

GhK3Ndf — 06/12/2024 8:15 PM thanks!

I did not mention any other potential mitigating factors from out-of-scope/external systems as the golang in scope for the assessment was only smartcontract_log_parser.go and ethereum_block_scanner (specifically the GetTxInITem function as per here in "Bifrost Overview") and out-of-scope components were subsequently not assessed. For example observe.go as raised in finding #22 which seems to have been referenced in relation to my finding #14 above.

I raised my issue #14 as medium based on the provided overview of Bifrost, which implied that GetTxInITem should be treated as a critical component:

The most important part of Bifrost for this audit contest is the smartcontract_log_parser, and specifically the GetTxInItem function. The GetTxInItem is run for each obvserve transaction on EVM chains. The function iterates through each log emitted by the transaction, and determines if any valid logs were emitted by the THORChain router. If it determines that a log was emitted by the THORChain router, it parses that log and forwards the appropriate details to the THORChain Layer 1 for processing. This is an incredibly sensitive and crucial piece of the codebase, as if it can be tricked, this means malicious contracts can trick Bifrost into informing THORChain of something that didn’t actually happen, leading to loss of funds. In fact, in July of 2021, THORChain’s old router + Bifrost were hacked in this exact way. Details of those hacks

[hGq2Wnl]

In the interest of clarity, this is what the sponsors told me about the TransferAllowance event, which corresponds to the transferAllowanceEvent case in smartcontract_log_parser:GetTxInItem after I (GhK3Ndf ) asked a very similar question in a private thread as shown in the warden's comment above:

GhK3Ndf — 06/12/2024 7:04 PM Hi (sponsors) ,apologies for a new thread, but is the TransferAllowance event only supposed to be emited once per block/batch of transactions, like with TransferOut? Referring to this comment: https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/bifrost/pkg/chainclients/shared/evm/smartcontract_log_parser.go#L245 Thanks! GhK3Ndf — 06/12/2024 8:13 PM (I tag additional sponsor ) olegpetrov — 06/12/2024 8:13 PM Eridanus know more about this than me! Eridanus (9R) — 06/12/2024 8:15 PM Correct, there should be only one TransferAllowance event per tx GhK3Ndf — 06/12/2024 8:15 PM thanks!

I did not mention any other potential mitigating factors from out-of-scope/external systems as the golang in scope for the assessment was only smartcontract_log_parser.go and ethereum_block_scanner (specifically the GetTxInITem function as per here in "Bifrost Overview") and out-of-scope components were subsequently not assessed. For example observe.go as raised in finding #22 which seems to have been referenced in relation to my finding #14 above.

I raised my issue #14 as medium based on the provided overview of Bifrost, which implied that GetTxInITem should be treated as a critical component:

The most important part of Bifrost for this audit contest is the smartcontract_log_parser, and specifically the GetTxInItem function. The GetTxInItem is run for each obvserve transaction on EVM chains. The function iterates through each log emitted by the transaction, and determines if any valid logs were emitted by the THORChain router. If it determines that a log was emitted by the THORChain router, it parses that log and forwards the appropriate details to the THORChain Layer 1 for processing. This is an incredibly sensitive and crucial piece of the codebase, as if it can be tricked, this means malicious contracts can trick Bifrost into informing THORChain of something that didn’t actually happen, leading to loss of funds. In fact, in July of 2021, THORChain’s old router + Bifrost were hacked in this exact way. Details of those hacks

(to be clear I am not disputing the new information provided - just providing clarity on why #14 was raised as it was. Thanks!)

[c4-judge]

trust1995 marked the issue as grade-b