Zellic - Audit Reports

Category: Coding Mistakes

Signature authenticator authentication bypass

Critical Severity

Critical Impact

High Likelihood

Description

For legacy support, the signature authenticator is used by default for any accounts without any registered authenticators. The signature authenticator implements the GetAuthenticationData handler for the Authenticator interface. The handler parses signers and signatures from the transaction and returns an indexed list of both the signers and the signatures.

However, the message index is cast to int8 before the handler is invoked:

authData, err := authenticator.GetAuthenticationData(neverWriteCacheCtx, tx, int8(msgIndex), simulate)
if err != nil {
	return ctx, err
}

This causes the cast to overflow, resulting in the message index becoming negative.

func GetSignersAndSignatures(
	msgs []sdk.Msg,
	suppliedSignatures []signing.SignatureV2,
	feePayer string,
	// we use the message index to get signers and signatures for
	// a specific message, with all messages.
	msgIndex int,
) ([]sdk.AccAddress, []signing.SignatureV2, error) {
    [...]
	// Iterate over messages and their signers.
	for i, msg := range msgs {
		for _, signer := range msg.GetSigners() {
            [...]
			// If dealing with a specific message, capture its signers.
			if specificMsg && i == msgIndex {
				resultSigners = append(resultSigners, signer)
			}

Since msgIndex is negative, specificMsg && i == msgIndex will never match. This causes GetSignersAndSignatures to return empty lists for signers and signatures.

Impact

Signature checks are skipped for transactions having more than 128 messages. This could allow an attacker to maliciously sign and execute any message --- for example, sending coins to themselves. They could simply add fake signature and signer info to the message, and it would get executed.

An example proof of concept (POC), which is located in the appendix ref↗, was provided to Osmosis Labs that demonstrates an attacker signing a message to transfer coins to themselves:

The POC will output the following:

Balances before:
hacker: amount: "139621170"
denom: uosmo
victim: amount: "99351536125"
denom: uosmo

{
  "msg_index": 128,
  "log": "",
  "events": [
    {
      "type": "coin_received",
      "attributes": [
        {
          "key": "receiver",
          "value": "osmo1d6aldupd067vm4807qvkcm20j5ts2nmhzwu4y7"
        },
        {
          "key": "amount",
          "value": "10000000uosmo"
        }
      ]
    },
    {
      "type": "coin_spent",
      "attributes": [
        {
          "key": "spender",
          "value": "osmo12smx2wdlyttvyzvzg54y2vnqwq2qjateuf7thj"
        },
        {
          "key": "amount",
          "value": "10000000uosmo"
        }
      ]
    },
    {
      "type": "message",
      "attributes": [
        {
          "key": "action",
          "value": "/cosmos.bank.v1beta1.MsgSend"
        },
        {
          "key": "sender",
          "value": "osmo12smx2wdlyttvyzvzg54y2vnqwq2qjateuf7thj"
        },
        {
          "key": "module",
          "value": "bank"
        }
      ]
    },
    {
      "type": "transfer",
      "attributes": [
        {
          "key": "recipient",
          "value": "osmo1d6aldupd067vm4807qvkcm20j5ts2nmhzwu4y7"
        },
        {
          "key": "sender",
          "value": "osmo12smx2wdlyttvyzvzg54y2vnqwq2qjateuf7thj"
        },
        {
          "key": "amount",
          "value": "10000000uosmo"
        }
      ]
    }
  ]
}

Balances after:
hacker: amount: "149608670"
denom: uosmo
victim: amount: "99341536125"
denom: uosmo

Recommendations

The int8 cast should be removed since it is not required.

Remediation

This issue has been acknowledged by Osmosis Labs, and a fix was implemented in commit 50eb8ae5↗. The int8 cast was removed for message indexes.