search

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

6 stars 3 forks source link

`Fund stuck forever` in vault in case of multiple deposits to different vaults #27

Closed howlbot-integration[bot] closed 4 months ago

howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

Description

While removing whitelisting will open the door for more composability with THORChain, the current SmartContractLogParser::GetTxInItem implementation seems too rigid to leverage such composability, at least in regard to the Deposit event, which seems to warrant High severity as user funds loss could occurs.

Imagine a dApp that integrates with THORChainRouter (as possible in the future with no whitelisting) decides to make multiple deposits in a single transaction which would be as follow:

EOA --> DepositAggregatorDapp::depositInBatch --> THORChainRouter::depositWithExpiry(Vault1, Asset1, ...)
                                              \-> THORChainRouter::depositWithExpiry(Vault1, Asset2, ...)
                                              \-> THORChainRouter::depositWithExpiry(Vault2, Asset2, ...)
                                              \-> THORChainRouter::depositWithExpiry(Vault3, Asset3, ...)

1) This would fall into the new path, as DepositAggregatorDapp is a smart contract call, but not directly the router. And since those events are generated by the router, it would pass the main gate.

        case evm.IsSmartContractCall(tx, receipt):
            // Tx to a different contract, attempt to parse with max allowable logs
            return e.getTxInFromSmartContract(tx, receipt, int64(e.cfg.MaxContractTxLogs))

2) Inside GetTxInItem, the first deposit event would be processed properly (depositWithExpiry(Vault1, Asset1, ...), but the 2nd event processed would fail the following condition, which would cause the function to abort and return an error. This is due to the fact that in a same transaction, there would be multiple deposit event that goes to different vaults, which granted doesn't make sense in a whitelisted world, but seems to make sense in a non-whitelisted world. This would means that those deposits event would not be accounted for, thus not sent to THORChain, which is effectively is a user funds loss for the user as those funds would be stuck there forever, like a donation.

            if len(txInItem.To) > 0 && !strings.EqualFold(txInItem.To, depositEvt.To.String()) {
                return false, fmt.Errorf("multiple events in the same transaction, have different to addresses , ignore")
            }
func (scp *SmartContractLogParser) GetTxInItem(logs []*etypes.Log, txInItem *types.TxInItem) (bool, error) {
    if len(logs) == 0 {
        scp.logger.Info().Msg("tx logs are empty return nil")
        return false, nil
    } else if int(scp.maxLogs) > 0 && len(logs) > int(scp.maxLogs) {
        scp.logger.Info().Msgf("tx logs are too many, ignore")
        return false, nil
    }
    isVaultTransfer := false
    for _, item := range logs {
        // only events produced by THORChain router is processed
        if !scp.addressValidator(&item.Address, false) { //@audit: does an attacker SM delegating to router would generate event from Router or smart contract attack?
            continue
        }
        earlyExit := false
        switch item.Topics[0].String() {
        case depositEvent:
            // router contract , deposit function has re-entrance protection
            depositEvt, err := scp.parseDeposit(*item)
            if err != nil {
                scp.logger.Err(err).Msg("fail to parse deposit event")
                continue
            }
            if len(depositEvt.Amount.Bits()) == 0 {
                scp.logger.Info().Msg("deposit amount is 0, ignore")
                continue
            }
            if len(txInItem.To) > 0 && !strings.EqualFold(txInItem.To, depositEvt.To.String()) {
                return false, fmt.Errorf("multiple events in the same transaction, have different to addresses , ignore")
            }
            if len(txInItem.Memo) > 0 && !strings.EqualFold(txInItem.Memo, depositEvt.Memo) {
                return false, fmt.Errorf("multiple events in the same transaction , have different memo , ignore")
            }
            asset, err := scp.assetResolver(depositEvt.Asset.String())
            if err != nil {
                scp.logger.Err(err).Str("token address", depositEvt.Amount.String()).Msg("failed to get asset from token address")
                continue
            }
            if asset.IsEmpty() {
                continue
            }
            txInItem.To = depositEvt.To.String()
            txInItem.Memo = depositEvt.Memo
            decimals := scp.decimalResolver(depositEvt.Asset.String())
            txInItem.Coins = append(txInItem.Coins,
                common.NewCoin(asset, scp.amtConverter(depositEvt.Asset.String(), depositEvt.Amount)).WithDecimals(decimals))
            isVaultTransfer = false

            ...

Impact

User deposits could be stuck forever into the THORChain vault(s) in case a non-whitelisted smart contract do multiple deposits to different vault in the same transaction.

Tools Used

Manual review

Recommended Mitigation Steps

The current event processing rules in GetTxInItem might need to evolve with the fact of removing the whitelisting. While in this report I showcased how an aggregator/integrator could break the Deposit event, other events would need also to be reviewed. Seems like a refactoring would be needed in GetTxInItem in order to support the use case described in my report, which seems realistic with increased composability.

Assessed type

Invalid Validation

the-eridanus commented 4 months ago

As pointed out the GetTxInItem (and in general Bifrost) is not meant to support multiple DepositEvents in one tx, and I'm not sure removing the whitelist changes that. This is a design decision of the protocol - if users submit a tx with multiple deposit events that would be considered user error.

the-eridanus commented 4 months ago

Added label: sponsor disputed. Not allowing multiple deposit events in a single tx is a design decision as called out in the function comments. Users interacting with the Router should not submit a tx with multiple depositWithExpiry calls - doing so would be an improper use of the protocol.

AsmundTC commented 4 months ago

Seconding what @the-eridanus said - even prior to the whitelist or any of the routerV5 changes covered in this program, there was nothing stopping someone from deploying their own contract that had a function that called into the deposit pathway on the Router more than once, and thereby emitting multiple deposit events for a single txn.

trust1995 commented 4 months ago

The issue shows a potential problem when a dApp integrates incorrectly with the contract in scope. We assume due diligence by integrators, so it remains OOS.

c4-judge commented 4 months ago

trust1995 marked the issue as unsatisfactory: Out of scope

dontonka commented 4 months ago

Not allowing multiple deposit events in a single tx is a design decision as called out in the function comments.

@the-eridanus could you link me those comments please? The only information I could find in that regards is the following text within an error:

            if len(txInItem.To) > 0 && !strings.EqualFold(txInItem.To, depositEvt.To.String()) {
                return false, fmt.Errorf("multiple events in the same transaction, have different to addresses , ignore")
            }

Understood, could you point me to the dev documentation explaining that limitation and risk? Or that would be the same information pointed in the previous point? 

even prior to the whitelist or any of the routerV5 changes covered in this program, there was nothing stopping someone from deploying their own contract that had a function that called into the deposit pathway on the Router more than once, and thereby emitting multiple deposit events for a single txn

True, I'm just not sure how long without whitelisting this have been in production. Nevertheless, it doesn't change the fact that a new dev could step in and behave like this, except if it's clearly indicated he should not do so otherwise funds would be stuck in official documentation. Furthermore, with the whitelisting removed (as you also point out), it will be much easier for a smart contract (nonReentrant would not help) todo such operation and seems a realistic use case, which is why I ultimately submitted this issue.

The issue shows a potential problem when a dApp integrates incorrectly with the contract in scope. We assume due diligence by integrators, so it remains OOS.

The exact same reasoning has been deemed acceptable for issue 25, so I would like to understand why the other issue was deemed satisfactory and not mine? They both share the same root cause:

dontonka — 06/09/2024 4:30 PM
12) what would be the impact if a Deposit event is properly made through the router, but Bifrost doesn't process the Deposit event properly (failing due to an error)?

sysrqb — 06/10/2024 9:00 AM
12) Bifrost failing to observe/process a Deposit event leads to a stuck transaction - the funds are still held on the EVM inbound chain, but thornode does not process the corresponding action and create an outbound event

dontonka — 06/10/2024 9:06 AM
How do you handle those situations? Does this means user funds are stuck forever in the vault (or router for the ERC20) and lost from the user perspective, like a donation which was not meant to be?

sysrqb — 06/10/2024 9:09 AM
yes from the user's perspective the funds are stuck on the network - in the case where correct usage of the network leads to stuck funds due to a bug, the fix is usually to correct the bug, then have a manual refund issued in the next network upgrade (can only be performed by 2/3rds of the validators adopting the new code)

Conclusion

While I definatelly agree with all of you guys that integrator require todo proper due diligence, the problem exposed in my report are two folds:

So based on those facts, combined that #25 was deemed valid, I would like the judge to reconsider the evaluation of this report.

the-eridanus commented 4 months ago

@dontonka I appreciate the call out, but the point is that having multiple deposit events in a single deposit tx is not something that is meant to or will be supported now or in the near future. All of THORChain's use cases are meant to be a single action at a time (liquidity deposit/withdraw, swap, open loan etc). The team will consider updating the documentation to call out this limitation, but this has not been a problem for us and we don't expect it to be a problem for us after the removal of the whitelist

trust1995 commented 4 months ago

The exact same reasoning has been deemed acceptable for https://github.com/code-423n4/2024-06-thorchain-findings/issues/25, so I would like to understand why the other issue was deemed satisfactory and not mine? They both share the same root cause:

They don't share a root cause or they would be dupped together. They share the assumption that the caller is using the platform in a way THORchain assumed it will not be used.

However #25 imo is more severe because a user using whitelisted tokens could get rekt, in the case of a whitelist a much lower assumption of compentency can be assumed for the caller. However, obviously in this case a user can be expected to be much more tech-savvy. Does that take make sense?

dontonka commented 4 months ago

They don't share a root cause or they would be dupped together. They share the assumption that the caller is using the platform in a way THORchain assumed it will not be used.

Yes you are right, thanks for correcting me, I meant same assumption.

However https://github.com/code-423n4/2024-06-thorchain-findings/issues/25 imo is more severe because a user using whitelisted tokens could get rekt, in the case of a whitelist a much lower assumption of compentency can be assumed for the caller. However, obviously in this case a user can be expected to be much more tech-savvy. Does that take make sense?

I agree on that with you, but while the current caller on my issue is more tech savy, if he doesn't have the tool to understand this limitation, that is also a problem too. It's like I'm asking a random person and a bucher to cut a piece of meat, but I'm not providing knife to any of them, the bucher will not be able to cut the piece of meat with his bare hands even if he knows really well how todo it with his knife. In the end both scenarios translate into the same likelihood to me:

25: Likelihood: noob user/bad integretor : Low Impact : Medium, as fund can be rescued. This translate in Low severity in the usualy matrix (I know in C4 we don't use that). But since this is clearly OOS, doesn't even qualify.

27: Likelihood: tech savy dev/but without proper tool to figure out his mistakes : Low Impact: High, as fund are stuck, and will require an upgrade to the protocol to unstuck. This translate in Medium severity.

@dontonka I appreciate the call out, but the point is that having multiple deposit events in a single deposit tx is not something that is meant to or will be supported now or in the near future. All of THORChain's use cases are meant to be a single action at a time (liquidity deposit/withdraw, swap, open loan etc).

That's loud and clear for you guys. Then this fall into the missing documentation category. Again, with the whitelisting removed, a dApp could decide to bundle up multiple deposits for a single user in a single transaction. Imagine a dApp that would allow to add liquidity for THORChain multiple assets at the time, the user simply put in their UX the amounts of each asset he desire to deposit, press Deposit button, and then the contract proceed calling those deposits in a single transaction, I don't think this is so far fetched. It was not possible with the whitelisting, but it would be after. It will be counter intuitive for the dApp dev to spot the issue as the event will be emitted successfully at the EVM level.

The team will consider updating the documentation to call out this limitation, but this has not been a problem for us and we don't expect it to be a problem for us after the removal of the whitelist

The documentation definatelly require to be added in that regards. What would be the best way for a dApp developer to see this problem BEFORE being in prod and having his funds stuck, would it be possible in testnet somehow?