HildisviniOttar/thorchain_vulnerability_affiliate.md

## thorchain_vulnerability_affiliate.md

      
    Raw
  

              thorchain_vulnerability_affiliate.md
            
          
    Description

THORChain accepts messages of type SWAP and ADD that contain affiliate fees.
Affiliate fees are defined in basis points, for example an affiliate fee of 100 (of 10000) is 1%.
Affiliate fees are indicated by MEMO as described in https://docs.thorchain.org/developers/transaction-memos
THORChain does not do any bounds checking on the affiliate fee percentage, which can result in payment of unlimited affiliate fees to an attacker.
Attack:

An attacker can craft a SWAP or ADD memo using their own address as benefactor of affiliate fees, with an unbounded affiliate fee (e.g. 1000000000000).

The attacker would have their own SWAP or ADD subtracted down to zero, but the Affiliate earns unbounded rewards.

These rewards can either dilute the pool shares (ADD) resulting in virtual unlimited ownership of the pool and ability to withdraw all assets, or an SWAP to drain RUNE and later perform a legal SWAP into assets to steal all of the assets.
The attack only requires a small amount of asset which amplifies the percentage. For example 10-100 RUNE and/or 0.1-1.0 ETH.
Funds at risk / Severity:

Critical
Up to 100% of all pools, depending on the speed of the attack and the response time in validators shutting down the network.
Existing Defences

The insolvency checker would not catch because the pool shares would be diluted from an unbounded ADD, resulting in "legitimate" ownership of 99% of pool shares, followed by a "legal" asymmetric withdrawal of most (all?) assets.
A global search for AffiliateBasisPoints shows no bounds checks.
Code walkthrough

The MSG comes in via existing methods and is hit with ValidateBasic which only checks that both an address and amount are specified. It does not do any bounds checks.
For SWAP, the message is rounted to x/thorchain/handler_observed_tx_in.go addSwapV1 which calculates the affiliate reward with GetShare():
if !msg.AffiliateBasisPoints.IsZero() && msg.AffiliateAddress.IsChain(common.THORChain) {
	amt = common.GetShare(
		msg.AffiliateBasisPoints,
		cosmos.NewUint(10000),
		msg.Tx.Coins[0].Amount,
	)
	msg.Tx.Coins[0].Amount = common.SafeSub(msg.Tx.Coins[0].Amount, amt)
}
The GetShare() function allows for values to be returned greater than 100% of the amount because it is used for various functions where this is required, for example to calculate pool ratio amounts in other parts of code.
A call to GetShare(100000000000000,10000, 10 RUNE) would result in amt being 100 billion RUNE. This is bounded only by the size of a 256 bit UInt. Let's call this "infinite".
This infinite amt is subtracted from the regular swap with SafeSub which will result in a value of zero for the regular swap.
The regular swap is added to the swap queue. If there is an error, it just prints and continues:
if err := h.mgr.Keeper().SetSwapQueueItem(ctx, msg, 0); err != nil {
	ctx.Logger().Error("fail to add swap to queue", "error", err)
}
Then we get to affiliate:
if !amt.IsZero() {
	affiliateSwap := NewMsgSwap(
		msg.Tx,
		common.RuneAsset(),
		msg.AffiliateAddress,
		cosmos.ZeroUint(),
		common.NoAddress,
		cosmos.ZeroUint(),
		msg.Signer,
	)
	affiliateSwap.Tx.Coins[0].Amount = amt

	if err := h.mgr.Keeper().SetSwapQueueItem(ctx, *affiliateSwap, 1); err != nil {
		ctx.Logger().Error("fail to add swap to queue", "error", err)
	}
}
This uses the attacker amt value and performs a RUNE SWAP to the attackers affiliate address.
For ADD liquidity it is similar. In handler_add_liquidity.go handleCurrent:
// add liquidity has an affiliate fee, add liquidity for both the user and their affiliate
affiliateRune := common.GetShare(msg.AffiliateBasisPoints, cosmos.NewUint(10000), msg.RuneAmount)
affiliateAsset := common.GetShare(msg.AffiliateBasisPoints, cosmos.NewUint(10000), msg.AssetAmount)
userRune := common.SafeSub(msg.RuneAmount, affiliateRune)
userAsset := common.SafeSub(msg.AssetAmount, affiliateAsset)
Similarly, affiliateRune and/or affiliateAsset can be infinite bounded. Firstly the zero userRune/userAsset is added to the pool, then the attackers poisoned affiliateRune/affiliateAsset is added with an unlimited amount, resulting in virtually 100% of pool ownership:
err = addLiquidity(
	ctx,
	msg.Asset,
	userRune,
	userAsset,
	msg.RuneAddress,
	msg.AssetAddress,
	msg.Tx.ID,
	stage,
	h.mgr.GetConstants(),
)
if err != nil {
	return err
}

err = addLiquidity(
	ctx,
	msg.Asset,
	affiliateRune,
	affiliateAsset,
	msg.AffiliateAddress,
	common.NoAddress,
	msg.Tx.ID,
	stage,
	h.mgr.GetConstants(),
)
I didn't test against TESTNET for fear of accidental disclosure, but walking through code I think the first one doesn't return error, so the second poisoned ADD (the hack) will occur here too.
How was it found?

I walked through the code in various handlers looking for where it performs calculations to add pool units, asking myself what if an attacker could control this value, what could they do.
I then discovered that an attacker could control the AffiliateBasisPoints variable through the MEMO field, and confirmed the exploit by reading code.
Disclosure

This was reported to @Heimdall on Discord at 1000h AEST, 27 Jul 2021. A video conference was organised with @Heimdall and @leena to go over the code and confirm it.
This vulnerability will not be publicly disclosed until 90 days or until fixed in THORChain with all ACTIVE validators running the patched code.
How to fix

After discussing with @heimdall and @leena, I have created two defensive Gitlab issues, which fix the bug from two angles:

Validate input affiliate fee <= 10% (also solves a business logic @leena has been meaning to implement), and
Implement a SafeGetShare function that does not allow returning more than 100% of the asset, then used by call sites that should never return values more than 100%.

These are written up here:

https://gitlab.com/thorchain/thornode/-/issues/1049

https://gitlab.com/thorchain/thornode/-/issues/1050
The network is currently offline with trading halted prior to shutdown. The fixes must be implemented prior to enabling trading.
It should not be a problem upgrading the network to the new code and still running old version temporarily whilst trading is halted.
All call sites that use AffiliateBasisPoints are in either Swap or Add Liquidity modules.
Bounty

I really hope it qualifies for the beer coolers: "I could have stolen 1000 BTC, but fuck centralised exchanges" ;)
Closing thoughts

Thanks to the team for quick comms and their professionalism handling this.