Zero value transfers are not ignored in log-parser.go when they should be ignored since Zero value transfers and approvals should revert according to the README

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/bifrost/pkg/chainclients/shared/evm/smartcontract_log_parser.go#L189-L192 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/bifrost/pkg/chainclients/shared/evm/smartcontract_log_parser.go#L213-L240 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/bifrost/pkg/chainclients/shared/evm/smartcontract_log_parser.go#L295-L334 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/bifrost/pkg/chainclients/shared/evm/smartcontract_log_parser.go#L252-L254

Vulnerability details

Impact

Transfer and Approve function according to the readme should revert on zero value transfer and approvals but the THORChain_Router fails to check this and relies on the Smartcontract_log_parser.go to ignore zero value transactions. The GetTXInItem fails to remove some transaction that should be ignored when we are swapping and when we are transferring. According to the README

Revert on zero value transfers Yes,

https://github.com/d-xo/weird-erc20?tab=readme-ov-file#revert-on-zero-value-transfers

AND

Revert on zero value approvals Yes

https://github.com/d-xo/weird-erc20?tab=readme-ov-file#revert-on-zero-value-approvals

Proof of Concept

The code implementation fails to check if the transferOut and ransferOutAndCallEvent event has an amount of zero, which allows for an elongation of calls with zero amount swaps and transfers. SafeApprove does not revert on zero approval.

function safeApprove(
    address _asset,
    address _address,
    uint _amount
) internal {
    (bool success, ) = _asset.call(
      abi.encodeWithSignature("approve(address,uint256)", _address, _amount)
    ); // Approve to transfer
    require(success);
}

case depositEvent:
    .................................................
            if len(depositEvt.Amount.Bits()) == 0 {
                scp.logger.Info().Msg("deposit amount is 0, ignore")
                continue
            }

also observe

case transferAllowanceEvent:
...........................................................
            if len(transferAllowanceEvt.Amount.Bits()) == 0 {
                scp.logger.Error().Msg("transfer allowance event with amount 0, ignore")
                continue
            }

These checks are used to sieve out transactions, but two other cases lack this checks thereby allowing anyone including an attacker to enlongate the GetTxInItem .Hence the GetTxitem fails to sieve out transaction that should be ignored.

case transferOutEvent:
            // it is not legal to have multiple transferOut event , transferOut event should be final
            transferOutEvt, err := scp.parseTransferOut(*item)
            if err != nil {
                scp.logger.Err(err).Msg("fail to parse transfer out event")
                continue
            }
            m, err := memo.ParseMemo(common.LatestVersion, transferOutEvt.Memo)
            if err != nil {
                scp.logger.Err(err).Str("memo", transferOutEvt.Memo).Msg("failed to parse transferOutEvent memo")
                continue
            }
            if !m.IsOutbound() && !m.IsType(memo.TxMigrate) {
                scp.logger.Error().Str("memo", transferOutEvt.Memo).Msg("incorrect memo for transferOutEvent")
                continue
            }
            asset, err := scp.assetResolver(transferOutEvt.Asset.String())
            if err != nil {
                return false, fmt.Errorf("fail to get asset from token address: %w", err)
            }
            if asset.IsEmpty() {
                return false, nil
            }

case transferOutAndCallEvent:
            transferOutAndCall, err := scp.parseTransferOutAndCall(*item)
            if err != nil {
                scp.logger.Err(err).Msg("fail to parse transferOutAndCall event")
                continue
            }
            scp.logger.Info().Msgf("transferOutAndCall: %+v", transferOutAndCall)
            m, err := memo.ParseMemo(common.LatestVersion, transferOutAndCall.Memo)
            if err != nil {
                scp.logger.Err(err).Msgf("fail to parse memo: %s", transferOutAndCall.Memo)
                continue
            }
            if !m.IsType(memo.TxOutbound) {
                scp.logger.Error().Msgf("%s is not an outbound memo", transferOutAndCall.Memo)
                continue
            }
            decimals := scp.decimalResolver(NativeTokenAddr)
            txInItem.Coins = common.Coins{
                common.NewCoin(scp.nativeAsset, scp.amtConverter(NativeTokenAddr, transferOutAndCall.Amount)).WithDecimals(decimals),
            }
            aggregatorAddr, err := common.NewAddress(transferOutAndCall.Target.String())
            if err != nil {
                scp.logger.Err(err).Str("aggregator_address", transferOutAndCall.Target.String()).Msg("fail to parse aggregator address")
                continue
            }
            aggregatorTargetAddr, err := common.NewAddress(transferOutAndCall.FinalAsset.String())
            if err != nil {
                scp.logger.Err(err).Str("final_asset", transferOutAndCall.FinalAsset.String()).Msg("fail to parse aggregator target address")
                continue
            }

            txInItem.To = transferOutAndCall.To.String()
            txInItem.Memo = transferOutAndCall.Memo
            txInItem.Sender = transferOutAndCall.Vault.String()
            txInItem.Aggregator = aggregatorAddr.String()
            txInItem.AggregatorTarget = aggregatorTargetAddr.String()
            if transferOutAndCall.AmountOutMin != nil {
                limit := cosmos.NewUintFromBigInt(transferOutAndCall.AmountOutMin)
                if !limit.IsZero() {
                    txInItem.AggregatorTargetLimit = &limit
                }
            }

Tool Used

Manual code analysis

Mitigation

To mitigate this issue, it is recommended to include a check in both the transferOutAndCallEvent event and transferOut Event event functions to ignore zero transfers. This will prevent unauthorized elongation of calls and ensure that only legitimate transactions are processed.

Assessed type

Error