`earlyExit` is not set to true in `transferAllowanceEvent` case in `smartcontract_log_parser:GetTxInItem`

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/bifrost/pkg/chainclients/shared/evm/smartcontract_log_parser.go#L244

Vulnerability details

Impact

The transferAllowanceEvent case in smartcontract_log_parser:GetTxInItem does not set the earlyExit flag to true, which is required to terminate the log parsing process if more than one of a particular event is included in a transaction.

Only one TransferAllowance event is expected to be emitted per transaction, as is also the case for the TransferOut event. The transferOutEvent case in smartcontract_log_parser:GetTxInItem sets the earlyExit flag to true to prevent additional TransferOut events from being included in the transaction block, however this flag is not set in the transferAllowanceEvent case.

This may result in multiple TransferAllowance events being included and processed within a single transaction. 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.

File://bifrost/pkg/chainclients/shared/evm/smartcontract_log_parser.go

func (scp *SmartContractLogParser) GetTxInItem(logs []*etypes.Log, txInItem *types.TxInItem) (bool, error) { 
    ... 
    for _, item := range logs {
        ...
        earlyExit := false
        switch item.Topics[0].String() {
        ...
        case transferOutEvent:
            // it is not legal to have multiple transferOut event , transferOut event should be final
            ,,,
            )
            earlyExit = true 
            isVaultTransfer = false
        case transferAllowanceEvent:
            // there is no circumstance , router will emit multiple transferAllowance event
            // if that does happen , it means something dodgy happened
            ...
            isVaultTransfer = false
        ...
        }
        if earlyExit {
            break
        }

    return isVaultTransfer, nil
    }    

Tools Used

VS Code

Recommended Mitigation Steps

Set earlyExit = true at the end of the transferAllowanceEvent case of smartcontract_log_parser:GetTxInItem:

File://bifrost/pkg/chainclients/shared/evm/smartcontract_log_parser.go

func (scp *SmartContractLogParser) GetTxInItem(logs []*etypes.Log, txInItem *types.TxInItem) (bool, error) { 
    ... 
    for _, item := range logs {
        ...
        earlyExit := false
        switch item.Topics[0].String() {
        ...
        case transferAllowanceEvent:
            ...
            earlyExit = true 
            isVaultTransfer = false~
        ...
        }
        if earlyExit {
            break
        }

    return isVaultTransfer, nil
    }    

Assessed type

Other

[the-eridanus]

I believe this is an issue and should be fixed

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as selected for report

[trust1995]

This may result in multiple TransferAllowance events being included and processed within a single transaction. 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.

@the-eridanus can you confirm how these unwanted impacts could occur due to the bug outlined?

[the-eridanus]

This may result in multiple TransferAllowance events being included and processed within a single transaction. 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.

@the-eridanus can you confirm how these unwanted impacts could occur due to the bug outlined?

Reviewing this one again, I think this can be categorized as low severity. The only way this could be an issue is if somehow the bifrost code was updated unintentionally to sign multiple transferAllowance events in one tx, which is not likely.

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

trust1995 marked the issue as grade-c