howlbot-integration[bot] commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/main/bifrost/observer/observe.go#L321 https://github.com/code-423n4/2024-06-thorchain/blob/main/bifrost/pkg/chainclients/shared/evm/smartcontract_log_parser.go#L295

Vulnerability details

Description

The current bounce back to vault & continue logic in the router function call transferOut* and transferOutAndCall* allow the posibility for griefing attack even currently with the whitelisting. Such attack can cause waste in CPU/RAM as the events in those function will still be emitted and successful (as native asset calls are not protected by the allowance mechanism), so Bifrost will observe them properly but will discard them just before sending them to THORChain. Nevertheless, once the whitelisting is removed, combined with the fact that transferOutAndCall is not implementing the exitEarly path, this griefing attack can be amplied as explained in my report, which seems to warrant Medium severity.

Imagine an attacker that do the following once the whitelisting is removed, so in a single transaction call transferOutAndCall multiple time to hit the max log limit (set to 50) and sends 1 wei as msg.value:

EOA --> AttackerSC::attackTransferOutAndCall --> THORChainRouter::transferOutAndCall()
                                             \-> THORChainRouter::transferOutAndCall()
                                             \-> THORChainRouter::transferOutAndCall()
                                             \-> THORChainRouter::transferOutAndCall()
                                             ...

1) aggregator : smart contract that do nothing, so aggregator.call will fail. 2) to : would be a smart contract that can't receive native asset specially deployed upfront by the attacker, so payable(to).send(_safeAmount) would fail. 3) so the fund will be returned back to the attacker (minus the gas wasted in this dummy transaction). 4) events TransferOutAndCall will be emitted successfully.

  function transferOutAndCall(
    address payable aggregator,
    address finalToken,
    address to,
    uint256 amountOutMin,
    string memory memo
  ) public payable nonReentrant {
    uint256 _safeAmount = msg.value;
    (bool erc20Success, ) = aggregator.call{value: _safeAmount}(
      abi.encodeWithSignature(
        "swapOut(address,address,uint256)",
        finalToken,
        to,
        amountOutMin
      )
    );
    if (!erc20Success) {
      bool ethSuccess = payable(to).send(_safeAmount); // If can't swap, just send the recipient the ETH
      if (!ethSuccess) {
        payable(address(msg.sender)).transfer(_safeAmount); // For failure, bounce back to vault & continue.
      }
    }

    emit TransferOutAndCall(
      msg.sender,
      aggregator,
      _safeAmount,
      finalToken,
      to,
      amountOutMin,
      memo
    );
  }

5) assuming the memo was properly submitted as parameter, Bifrost (in SmartContractLogParser::GetTxInItem) will observe those events successfully (all of them, not only one), and try to send it all the way to THORChain but they will be discarded just before in Observer::filterObservations as the following condition will fail since the call is not coming from the vault, but from the attacker contract.

if ok, cpi := o.pubkeyMgr.IsValidPoolAddress(txInItem.Sender, chain); ok {

Impact

Griefing attack amplified against transferOutAndCall once the whitelisting is removed could DoS Bifrost or at least make it running hot.

Tools Used

Manual review

Recommended Mitigation Steps

Implement also the early exit path for transferOutAndCallEvent as you did for transferOutEvent. You might also want to constraint further calls to transferOut* in the router and not allow them to be called by anyone.

Furthermore, I consider the current implementation of the exit early path to be incomplete if the requirement is the following comment:

        case transferOutEvent:
            // it is not legal to have multiple transferOut event , transferOut event should be final

The current implementation will break the loop and exit at soon as a transferOutEvent is encounter yes, but what if there were more transferOutEvent afterward, should not that fact invalidate all this transaction's events as a whole as this would mean the transaction is not legetimate and breaking a key invariant, why would you consider the first event valuable for such transaction? Food for thoughts, but I would recommend to implement it a bit more robust, such that in the scenario reported here, all those events would be discarded in the parser.

Assessed type

DoS

the-eridanus commented 3 months ago

I agree that this is a problem - transferOutAndCall should only allow one event per tx. I will note that we intend to implement a batchTransferOut function that will allow multiple transferOut events per tx, but we will need to consider the DoS risk as outlined here.

c4-judge commented 3 months ago

trust1995 marked the issue as satisfactory

c4-judge commented 3 months ago

trust1995 marked the issue as selected for report

hGq2Wnl commented 2 months ago

Hi trust1995, believe this finding may be low/QA severity rather than medium.

This finding #22 appears to be that the host running the smartcontract_log_parser.go log parser will ultimately experience a "waste in CPU/RAM" or cause Bifrost to "run hot" after a case in an out-of-scope Bifrost component (observe.go) causes invalid transferOutAndCallEvents to be rejected. Since the attacker would need to spend gas fees as mentioned in the finding, this is unlikely to be profitable or effective in reality, nor has a concrete PoC been provided outlining how DoS would occur.

However finding #24 from the same warden and finding #18 from another warden have been marked as QA due to raising a similar off-chain DOS impact, with the worst case being inefficient code/slight performance hit outlined for finding #18 and a similar result for #24. It is not clear how this finding #22 differs from #18 or #24 in terms of code inefficiency and related consequences.

Additionally, new information relating to finding #14 (which references the same missing earlyExit path root cause but applied to the transferAllowanceEvent event) provided by the warden's comment on it here and the sponsor's follow up here suggest that a lack of earlyExit cases for events alone are low-risk issues rather than medium.

Please advise, thanks,

trust1995 commented 2 months ago

@the-eridanus please advise your thoughts, I've seen several cases where sponsor confirmed was used when severity wasn't aligned.

dontonka commented 2 months ago

I totally understand the motivation that now all contest's warden desire to kill the unique Med finding of this contest, but let's try to be based on facts. While I didn't want to intervene, I still wants todo some clarification, so judge and sponsor can take their final call on this one.

This finding https://github.com/code-423n4/2024-06-thorchain-findings/issues/22 appears to be that the host running the smartcontract_log_parser.go log parser will ultimately experience a "waste in CPU/RAM" or cause Bifrost to "run hot" after a case in an out-of-scope Bifrost component (observe.go) causes invalid transferOutAndCallEvents to be rejected. Since the attacker would need to spend gas fees as mentioned in the finding, this is unlikely to be profitable or effective in reality, nor has a concrete PoC been provided outlining how DoS would occur.

Let's not confuse things. First, the root cause of the grief attack it totally in scope and originating from the Router and how the event ingestion mechanism is lacking protection (the parser which is also in-scope) and could be amplified when the whitelisting is removed, this is the noveltly of this finding. The fact that I point out observe.go is just to make the reader understand that I made my investigation in depth, it means it reach until that part of the Bifrost code which is still far, but if those event would have reach THORChain node that would have warranted a High, as poluting THORChain itself.

Second, while I state in the impact about DoS, the issue title and impact is related to grief attack, which is the main point here, which are not meant to be always profitable, but to grief the protocol or users of this protocol even if not getting any rewards out of it. The fact that an attacker can exploit the system in a way that is not meant to be todo some grief (in this case wasting CPU/RAM/DiskSpace and maybe pollute some BiFrost metrics) seems to warrant the Medium severity. Also note that the gas cost on BSC or AVAX would be a fraction of ETH.

However finding https://github.com/code-423n4/2024-06-thorchain-findings/issues/24 from the same warden and finding https://github.com/code-423n4/2024-06-thorchain-findings/issues/18 from another warden have been marked as QA due to raising a similar off-chain DOS impact, with the worst case being https://github.com/code-423n4/2024-06-thorchain-findings/issues/18#issuecomment-2182469477/https://github.com/code-423n4/2024-06-thorchain-findings/issues/18#issuecomment-2187082922 outlined for finding https://github.com/code-423n4/2024-06-thorchain-findings/issues/18 and a similar result for https://github.com/code-423n4/2024-06-thorchain-findings/issues/24.

Indeed both of those were downgraded to QA. I'm not sure how you can say both resulted in similar DoS impact, 24 was clearly showing a ineffeciency and a potential DoS, much more impactfull then 18 from my perspective. While I can agree that the CPU/RAM/Diskspace waste might be not dramatically high (and will be relative to how high maxLog cap is set and how much an attacker is willing to spend), the point here is that this can be exploited by an external actor and the magnitud is unknown, it's not something dormant in the code itself like 14, 18, and 24.

Additionally, new information relating to finding https://github.com/code-423n4/2024-06-thorchain-findings/issues/14 (which references the same missing earlyExit path root cause but applied to the transferAllowanceEvent event) provided https://github.com/code-423n4/2024-06-thorchain-findings/issues/103#issuecomment-2187048410 and the sponsor's follow up https://github.com/code-423n4/2024-06-thorchain-findings/issues/103#issuecomment-2189294834 suggest that a lack of earlyExit cases for events alone are low-risk issues rather than medium.

That was only for transferAllowance, not transferOutAndCall and not with the twist added here when the whitelist is removed, so you need to put it in proper context.

14si2o commented 2 months ago

@dontonka

Could you elaborate how exactly users or the protocol would suffer under the supposed grief attack?

From what you've said, the only possible impact is a slight waste in CPU/RAM while the attack has a definite cost to the attacker.

Unless you can demonstrate that there is a reasonable scenario under which this causes a loss to users/protocol, I would have to agree with @hGq2Wnl that this does not reach the impact required for a Medium finding.

dontonka commented 2 months ago

Wait a minute so both of you argumenting here are not even participant in this contest an neither C4 staff, is that accurate 🤔? This is the reason why C4 should introduce fees to participate in escalation.

Could you elaborate how exactly users or the protocol would suffer under the supposed grief attack?

The attack is explained in the report, please read it.

From what you've said, the only possible impact is a slight waste in CPU/RAM while the attack has a definite cost to the attacker.

Yes, also disk usage (as log are being printed) and maybe metrics (and other aspect which @the-eridanus could comment on if that is the case). Another aspect is that this could introduce some delay to legetimate transaction being observed and processed, depending on the magnitud of the attack, again this is unknown, relative to maxlog size, while cost would be minimal on Avax and BSC and will ultimatelly depends how much an attacker is willing to spend.

Unless you can demonstrate that there is a reasonable scenario under which this causes a loss to users/protocol, I would have to agree with @hGq2Wnl that this does not reach the impact required for a Medium finding.

Wait are you a judge? Is @hGq2Wnl also a judge? We can debate all day, but I just given more details in this and my previous post on the impact which should be enough for the judge and sponsor to take a decision, otherwise I will wait for them to ask me directly.

Let's read the severity categorization as follow. I will let the judge and sponsor come into agreement, but from my perspective, this fall much more into Med than QA.

QA (Quality Assurance) Includes Low risk (e.g. assets are not at risk: state handling, function incorrect as to spec, issues with comments) and Governance/Centralization risk (including admin privileges). Excludes Gas optimizations, which are submitted and judged separately. Non-critical issues (code style, clarity, syntax, versioning, off-chain monitoring (events, etc) are discouraged.

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

14si2o commented 2 months ago

Not sure what you're hoping to achieve with personal attacks? You can't see team members in data which is why u can't find us.

So you're saying yourself the impact as most is inefficient code for a definite cost of attack. And yes, let the judge & sponsor decide :)

dontonka commented 2 months ago

Not sure what you're hoping to achieve with personal attacks?

I'm sorry if you felt personally attacked by me wondering if you guys were participant in this contest or if you guys were a judge, which seems a legetimate question to ask. I'm gonna to have to watch my wording lol.

ShaheenRehman commented 2 months ago

This report looks invalid to me or a low severity issue at best. The warden makes a very wrong assumption about the code:

aggregator: smart contract that do nothing, so aggregator.call will fail.

This is not possible. Attacker cannot input any aggregator address. So how he can make sure that aggregator fails? There is a specific check in thornode which make sures only valid/whitelisted aggreagator address is inputted: https://github.com/code-423n4/2024-06-thorchain/blob/e5ae503d0dc2394a82242be6860eb538345152a1/x/thorchain/handler_swap.go#L121

Aggregator failing is a very less likelihood thing. Which means that this issue have a very low chance of happening and low/med impact. Even if the impact is med, the issue's severity still will be low

Hopefully sir @the-eridanus can confirm this & Hope it helps Judge Sahab @trust1995

Thanks!

dontonka commented 2 months ago

This is not possible. Attacker cannot input any aggregator address. So how he can make sure that aggregator fails? There is a specific check in thornode which make sures only valid/whitelisted aggreagator address is inputted:

With all due respect I think you are misunderstanding, but happy to be corrected by @the-eridanus if you are right. The transferOutAndCall has no access control and aggregator is coming into parameter, so this can be all forged and fake as my report is explaining. So the event would be emitted as described in my report and observed to the point indicated in my report.

This is not possible. Attacker cannot input any aggregator address. So how he can make sure that aggregator fails? There is a specific check in thornode which make sures only valid/whitelisted aggreagator address is inputted: https://github.com/code-423n4/2024-06-thorchain/blob/e5ae503d0dc2394a82242be6860eb538345152a1/x/thorchain/handler_swap.go#L121

What you are pointing at is THORchain node territory, which the event will not reach as Bifrost will discard it just before sending it to THORchain (again as pointed out in my report) as it will detect it is not being sent by THORChain node, so not legitimate.

ShaheenRehman commented 2 months ago

Interesting behavior you are mentioning, if that is the case then this finding deserves to be accepted as a Med but from my understanding you are assuming that wrong sir. Yea only sponsors can confirm the correct behavior now @the-eridanus

dontonka commented 2 months ago

I would like to add another fact about transferAllowance event (since my report is being compared with 14 which was downgraded to QA), this is not actually exploitable as the current report and transferOutAndCall since this call is protected by the _vaultAllowance, so the attacker would need to have some ERC20 deposited already, as otherwise _vaultAllowance[msg.sender][_asset] -= _amount; would revert, so it's a very different story.

  function transferAllowance(
    address router,
    address newVault,
    address asset,
    uint amount,
    string memory memo
  ) external nonReentrant {
    if (router == address(this)) {
      _adjustAllowances(newVault, asset, amount);
      emit TransferAllowance(msg.sender, newVault, asset, amount, memo);
    } else {
      _routerDeposit(router, newVault, asset, amount, memo);
    }
  }

  function _adjustAllowances(
    address _newVault,
    address _asset,
    uint _amount
  ) internal {
    _vaultAllowance[msg.sender][_asset] -= _amount;
    _vaultAllowance[_newVault][_asset] += _amount;
  }

hGq2Wnl commented 2 months ago

@dontonka my comment was more geared towards #24 and #18. Finding #14 does not mention any kind of further potential resource usage as that I did not deem that a valid finding for the same reasons outlined in the responses to #24 and #18.

14 was mentioned to highlight that in absence of other proven impact, the missing `earlyExit` flag appears to be considered low/QA risk. To be clear, the concern is that it is not clear in this finding #22, or any follow up comment so far, how a potential for slightly increased CPU/RAM/disk cycle usage in the log parser impacts the protocol or its users.

I am curious to see how findings like this are ultimately considered by the judges and sponsors. Thank you.

the-eridanus commented 2 months ago

Given that the worst case scenario for this bug is wasted CPU I believe it should be low/QA severity. Any grieving attack would require a lot of gas expenditure on the attacker's part, so I'm not really sure this would even be something attempted. That being said it should be fixed, hence an award of QA/low

c4-judge commented 2 months ago

trust1995 changed the severity to QA (Quality Assurance)

c4-judge commented 2 months ago

trust1995 marked the issue as grade-c

trust1995 commented 2 months ago

As sponsor confirmed, we are not able to attribute H/M severity to issues that only affect off-chain loads.

dontonka commented 2 months ago

@trust1995

I hope my QA report will get re-evaluated and elegible for a top spot since:

All my issues have been downgraded to QA (with proper justification so all good but dont ask me if I agree XD, still awaiting on #27 but I guess it will receive the same medecine), which bring unique value to the sponsor, probably more then the rest of the reports.
@the-eridanus did tag my QA report with Sponsor Confirmed (only to mine) which meant to me it was his way of indicating it was his preferred report (or was this random?) Meanwhile, @trust1995 you attributed it the worst grade.

Also if you could please remove the unsatisfactory label from my downgraded QA issues that would be appreciated, as from my understanding this override every other label and means no awards. So #24 and the current one.

dontonka commented 2 months ago

we are not able to attribute H/M severity to issues that only affect off-chain loads.

And for the record, this finding checks all the boxes of the C4 Med definition from my perspective, let me know which box is not checking on your end.

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Assets not at direct risk (✅ )
Function of the protocol or its availability could be impacted (✅) : yes CPU/Memory/Disk/Delay of legetimate txs, granted most likely minor, it's unknown, where do you trace this line exactly 🤔?
Leak value with a hypothetical attack path with stated assumptions (✅): yes, an unlegitimate event is being processed further then it should, which has the potential to be harmful. The protocol got hack twice for things in a similar pattern, and this contest was especially for this kind of bugs by opening the whitelisting, yet this is not deemed valid.
External requirements (✅): yes, externally trigger by an attacker at any moment.

If we use the usual matrix, we would get the following: Likelihood: High : since this can be done by anyone at any time. Impact: Low : so far the impact has not prooven to be higher then this. Severity: Medium

So looking at it in a way or another from my perspective it's seems a clean Medium, but hey, I don't think I can spend more energy on this one.

dontonka commented 2 months ago

I would like to add additional information to this report @trust1995 and @the-eridanus. I added this simple test to showcase the attack.

Test

We can see how all the information can be forged and sending a 0 msg.value would cost only 13402 gas to have one event emitted.

On BSC, the gas price is 1 Gwei. BNB price is let say 500 USD. This means it cost the attacker 0.000013402 BNB to generate one event, which means translate to 0.006701 USD. Imagine in a bear market and the price of BNB is a fraction of this, let's say 100 USD, that translate to 0.0013402 USD. Does the cost wasted on Bifrost in CPU/RAM/Disk space and potentially the delay this could cause against legetimate txs (in case of a huge attack) would be worth this price? I don't think anyone here can deny this possibility for sure, which fall into the Medium definition clearly. Furthermore, for this single price, the waste need to be multiplied by all the Bifrost nodes (I think there are at least 100, as every node will need todo this processing),

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console} from "forge-std/Test.sol";
import {THORChain_Router} from "../src/THORChain_Router.sol";

contract THORChain_RouterTest is Test {
    THORChain_Router public tr;

    function setUp() public {
        tr = new THORChain_Router();
    }

    function test_Attack() public {
        address emulateAVault = address(0xE6c10BC93953897d65F5c3B8587d7f7651950e58);
        tr.transferOutAndCall(
            payable(address(0x2)), // will fail, not a real smart contract
            address(0),            // emulating eth
            ethVault,              // mocking a real vault
            10 ether,              // this can be faked 
            "OUT:18C5B5A3EBA189734D5DC6AD7379173E81377AC6B47397691D027CA728AD00D5"       // fake but syntax good
        );
    }
}

Ran 1 test for test/THORChain_Router.t.sol:THORChain_RouterTest
[PASS] test_Attack() (gas: 13402)
Traces:
  [13402] THORChain_RouterTest::test_Attack()
    ├─ [10881] THORChain_Router::transferOutAndCall(0x0000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000, 0xE6c10BC93953897d65F5c3B8587d7f7651950e58, 10000000000000000000 [1e19], "assume_this_is_a_valid_memo")
    │   ├─ [108] PRECOMPILES::sha256(0x48c314f40000000000000000000000000000000000000000000000000000000000000000000000000000000000000000e6c10bc93953897d65f5c3b8587d7f7651950e580000000000000000000000000000000000000000000000008ac7230489e80000)
    │   │   └─ ← [Return] 0x5ce26322687d5c925a78fd771998b68418e2a35c22247553ba0cd6a89332fa27
    │   ├─ emit TransferOutAndCall(vault: THORChain_RouterTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], target: 0x0000000000000000000000000000000000000002, amount: 0, finalAsset: 0x0000000000000000000000000000000000000000, to: 0xE6c10BC93953897d65F5c3B8587d7f7651950e58, amountOutMin: 10000000000000000000 [1e19], memo: "OUT:18C5B5A3EBA189734D5DC6AD7379173E81377AC6B47397691D027CA728AD00D5")
    │   └─ ← [Stop]
    └─ ← [Stop]

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 419.84µs (52.35µs CPU time)

Now the cherry on the cake, it seems my initial assumption was wrong where this would be caught. Those 2 conditions in this function essentially identify if the tx is inbound or outbound. I was thinking the transaction would be dropped on the floor because of the first condition, and it will, but it's possible to make the second condition pass, without any cost as shown in my test, if the attacker put the official vault as the To AND send zero msg.value, the event is still emitted, parser will let this goes in, and here it will hit the 2nd condition and will work (so this outbound would be added thinking it's an inbound), so essentially will send this outbound transaction to THORChain from my understanding, after that I am unsure on the Impact.

func (o *Observer) filterObservations(chain common.Chain, items []types.TxInItem, memPool bool) (txs []types.TxInItem) {
    for _, txInItem := range items {
        // NOTE: the following could result in the same tx being added
        // twice, which is expected. We want to make sure we generate both
        // a inbound and outbound txn, if we both apply.

        isInternal := false
        // check if the from address is a valid pool
        if ok, cpi := o.pubkeyMgr.IsValidPoolAddress(txInItem.Sender, chain); ok {
            txInItem.ObservedVaultPubKey = cpi.PubKey
            isInternal = true

            // skip the outbound observation if we signed and manually observed
            if !o.signedTxOutCache.Contains(txInItem.Tx) {
                txs = append(txs, txInItem)
            }
        }
        // check if the to address is a valid pool address
        // for inbound message , if it is still in mempool , it will be ignored unless it is internal transaction
        // internal tx means both from & to addresses belongs to the network. for example migrate/consolidate
        if ok, cpi := o.pubkeyMgr.IsValidPoolAddress(txInItem.To, chain); ok && (!memPool || isInternal) {
            txInItem.ObservedVaultPubKey = cpi.PubKey
            txs = append(txs, txInItem)
        }
    }
    return
}

        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
                }
            }
        }

So based on the 2 pieces information added, the risk of grief could be greater than what was anticipated, as the cost seems minimal to generated a positive event, PLUS this seems to reach THORChain, which has the potential to cause more damage, so this seems to definatelly justify the Medium severity, and if there would be more impact prooven in THORchain, we could potentially get into High territory, but I'm just unsure on the impact yet.

The maxlog value set would not have much impact for this grief attack in the end, either set to 1, 10 or 50, the attacker smart contract would simply cap his loop to that value as his cost would not be really affected, it boils down a the cost per event emitted which I just exposed more in details.

trust1995 commented 2 months ago

As already explained, in audit contests it is very hard for impact of CPU load / memory load to be accepted. If there would be a null dereference or other panic shown, I would accept it, but simply heavy load arguments are too remote for the contract side which is the focus of contests.

This is something we'd like to revisit at a Supreme Court session as standardizing the judging course of action would be best for all.

dontonka commented 2 months ago

As already explained, in audit contests it is very hard for impact of CPU load / memory load to be accepted. This is something we'd like to revisit at a Supreme Court session as standardizing the judging course of action would be best for all.

You guys just mention for this impact it's not worth Medium, not that in the context of audit contest it is very hard to accept those type of impact, but good to know. There is definatelly a grey area in how those issue are being judged which needs to be clarified further such that the current situation are minimized in the future for the good and time of everyone.

If there would be a null dereference or other panic shown

Well in such case it would fall in High territory as it would mean an attacker would be able to DoS Bifrost on-demand. Similarly, if I could proove that this attack could be acheivable for an acceptable cost to DoS the Bifrost node, that would be also in High terriroty. The fact that I cannot fully proove this, but that there is definatelly an unauthorized and unexpected behavior which will cause the impact indicated, is why this should fall into Medium to me and why I submit it in the first place.

Here we have an attacker that can on-demand create a fake event (so by-pass for security measure at the contract level) for low cost, then by-pass the Bifrost security measure too at the parser level, and with the last information indicated, most likely would be able to reach THORChain (using the official vault as To as a vault and msg.value == 0 or 1 wei to minimize attacker cost) so by-passing the last Bifrost security wall before reaching THORChain, which would probably simply discard it at some point without causing major damage, but that is currently unknown. The fact that I'm using the vault as the To would means that bool ethSuccess = payable(to).send(_safeAmount); would work, so funds would be transfered to the vault but in an unexpected way as the To is never meant to be the Vault on those calls, so I'm not sure by donation if that is not screwing up the accounting in THORChain. @the-eridanus should definatelly give this an internal check with his team.

dontonka commented 2 months ago

Please close this ticket as I don't want to spend more time in this black hole and neither do you. In the end, even if what I mention is true, that is new information which was not indicated initially and which should not be rewarded. I will enjoy my zero rewards total for this contest and mark it in my agenda as a contribution for good will.

Have a great week-end!

dontonka commented 2 months ago

For the sponsor.

The donation attack I just described would most likely be caught here in THORChain, as the fake outbound tx would be observed initially as an outbound by Bifrost (in the parser) but then at the end Bitfros will send it as an inbound tx to THORChain, which in the end is not dramatic and doesn't seems to cause any accounting issues, by chance.

c4-judge commented 2 months ago

trust1995 marked the issue as grade-b

code-423n4 / 2024-06-thorchain-findings

Bifrost `risk of griefing attack` due to missing early exit path for `transferOutAndCall` #22

Lines of code

Vulnerability details

Description

Impact

Tools Used

Recommended Mitigation Steps

Assessed type

Test