howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

The GetTxInItem is returning two values (bool, error), this bool value is the variable isVaultTransfer we can see this in the code(see the arrow below):

func (scp *SmartContractLogParser) GetTxInItem(logs []*etypes.Log, txInItem *types.TxInItem) (bool, error) {
    ...
    isVaultTransfer := false
    for _, item := range logs {
        ...
        case depositEvent:
            ...
            isVaultTransfer = false
        case transferOutEvent:
            ...
            earlyExit = true
            isVaultTransfer = false
        case transferAllowanceEvent:
            ...
            isVaultTransfer = false
        case vaultTransferEvent:
            // TODO vault transfer events were only fired by ygg returns
            continue
        case transferOutAndCallEvent:

            ...
        }
        if earlyExit {
            break
        }
    }
    return isVaultTransfer, nil <-------
}

[Link].

The problem is that the functions is returning false event when the log was a Vault transfer which is wrong.

Impact

The GetTxInItem function is returning a wrong value when a vaultTransferEvent happened, since the node is not in the scope im taking it as a black box. But bassicly the node can not know if a log is vaultTransferEvent reading from the smartcontract_log_parser.

Proof of Concept

Taking out everything that we don't need to understand the issue, we can see more clearly that the GetTxInItem is returning a wrong bool when a vaultTransferEvent happen:

func (scp *SmartContractLogParser) GetTxInItem(logs []*etypes.Log, txInItem *types.TxInItem) (bool, error) {
    ...
    isVaultTransfer := false
    for _, item := range logs {
        ...
        case depositEvent:
            ...
            isVaultTransfer = false
        case transferOutEvent:
            ...
            earlyExit = true
            isVaultTransfer = false
        case transferAllowanceEvent:
            ...
            isVaultTransfer = false
        case vaultTransferEvent:
            // TODO vault transfer events were only fired by ygg returns
            continue <--------
        case transferOutAndCallEvent:

            ...
        }
        if earlyExit {
            break
        }
    }
    return isVaultTransfer, nil <-------
}

[Link].

See first arrow in the code snipped above. the vaultTransferEvent is just continue instead of set the isVaultTransfer to true.

Tools Used

Manual

Recommended Mitigation Steps

Consider set the isVaultTransfer to true in vaultTransferEvent case:

func (scp *SmartContractLogParser) GetTxInItem(logs []*etypes.Log, txInItem *types.TxInItem) (bool, error) {
    ...
    isVaultTransfer := false
    for _, item := range logs {
        ...
        case depositEvent:
            ...
            isVaultTransfer = false
        case transferOutEvent:
            ...
            earlyExit = true
            isVaultTransfer = false
        case transferAllowanceEvent:
            ...
            isVaultTransfer = false
        case vaultTransferEvent:
            // TODO vault transfer events were only fired by ygg returns                 
                        isVaultTransfer = true  <------
            continue 
        case transferOutAndCallEvent:

            ...
        }
        if earlyExit {
            break
        }
    }
    return isVaultTransfer, nil 
}

Assessed type

Other

the-eridanus commented 4 months ago

Adding label: sponsor disputed: this is not actually an issue as the vaultTransferEvent is deprecated and not used in the system.

AsmundTC commented 4 months ago

To add more context here, the vaultTransferEvent was only used by Yggdrasil vaults, which have long been removed from both the network and the codebase.

There are still certain remnants left in the codbase, such as the comment right above the continue that indirectly addresses this: // TODO vault transfer events were only fired by ygg returns

The implication of this TODO is to remove this at some point, as it is a dead code path. The comment could be clearer, but this is not a pathway that can be triggered any longer.

trust1995 commented 4 months ago

Since realistic Impact has not been demonstrated, will mark as QA. If there is a counterexample, warden is invited to demonstrated a PoC of it.

c4-judge commented 4 months ago

trust1995 changed the severity to QA (Quality Assurance)