Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix operations effects when signer changes. #2136

Closed
abuiles opened this issue Jan 20, 2020 · 2 comments · Fixed by #2375
Closed

Fix operations effects when signer changes. #2136

abuiles opened this issue Jan 20, 2020 · 2 comments · Fixed by #2375
Assignees
Labels
Milestone

Comments

@abuiles
Copy link
Contributor

abuiles commented Jan 20, 2020

Every time there is a signer change on an account with multiple signers, we are incorrectly reporting a signer updated effect on the other unaffected signers.

For example, the following operation https://horizon.stellar.org/operations/71491809406156801/effects is showing three effects, however, if we look at the XDR representation for the operation.

      { sourceAccount = GCHHFZV3NDPRHPKK2OVJI36CNLOOJUY5BX7HQD4OMC2NHCLQ6YHYF5WC,
        body = {
          type = SET_OPTIONS,
          setOptionsOp = {
            inflationDest = NULL,
            clearFlags = NULL,
            setFlags = NULL,
            masterWeight = NULL,
            lowThreshold = NULL,
            medThreshold = NULL,
            highThreshold = NULL,
            homeDomain = NULL,
            signer = {
              key = {
                type = SIGNER_KEY_TYPE_ED25519,
                ed25519 = ed43b19ae7eb0e1a3f28d3ffe9e3aa27e5a4509f4b1548910f6d26b377d94525
              },
              weight = 0
            }
          }
        } },

It is only removing one signer and the weight for the other signers is kept unchanged.

See full Tx Envelope
TransactionEnvelope = {
  tx = {
    sourceAccount = GDW6N6JLDMZJJGMYKZAHWDFCJXX3QNP6EEMTJ5PSHKTJBAVERLXNOJ32,
    fee = 400,
    seqNum = 70990698391797762,
    timeBounds = {
      minTime = 1520266282,
      maxTime = 0
    },
    memo = {
      type = MEMO_NONE
    },
    operations = [
      { sourceAccount = GCHHFZV3NDPRHPKK2OVJI36CNLOOJUY5BX7HQD4OMC2NHCLQ6YHYF5WC,
        body = {
          type = SET_OPTIONS,
          setOptionsOp = {
            inflationDest = NULL,
            clearFlags = NULL,
            setFlags = NULL,
            masterWeight = NULL,
            lowThreshold = NULL,
            medThreshold = NULL,
            highThreshold = NULL,
            homeDomain = NULL,
            signer = {
              key = {
                type = SIGNER_KEY_TYPE_ED25519,
                ed25519 = ed43b19ae7eb0e1a3f28d3ffe9e3aa27e5a4509f4b1548910f6d26b377d94525
              },
              weight = 0
            }
          }
        } },
      { sourceAccount = GCHHFZV3NDPRHPKK2OVJI36CNLOOJUY5BX7HQD4OMC2NHCLQ6YHYF5WC,
        body = {
          type = SET_OPTIONS,
          setOptionsOp = {
            inflationDest = NULL,
            clearFlags = NULL,
            setFlags = NULL,
            masterWeight = NULL,
            lowThreshold = NULL,
            medThreshold = NULL,
            highThreshold = NULL,
            homeDomain = NULL,
            signer = {
              key = {
                type = SIGNER_KEY_TYPE_HASH_X,
                hashX = 762329959e01e9716a7008835bc3944361d4b87d76f741ea60a167f28dc99107
              },
              weight = 0
            }
          }
        } },
      { sourceAccount = GCHHFZV3NDPRHPKK2OVJI36CNLOOJUY5BX7HQD4OMC2NHCLQ6YHYF5WC,
        body = {
          type = SET_OPTIONS,
          setOptionsOp = {
            inflationDest = NULL,
            clearFlags = NULL,
            setFlags = NULL,
            masterWeight = NULL,
            lowThreshold = NULL,
            medThreshold = NULL,
            highThreshold = NULL,
            homeDomain = NULL,
            signer = {
              key = {
                type = SIGNER_KEY_TYPE_ED25519,
                ed25519 = ede6f92b1b3294999856407b0ca24defb835fe211934f5f23aa69082a48aeed7
              },
              weight = 1
            }
          }
        } },
      { sourceAccount = GCHHFZV3NDPRHPKK2OVJI36CNLOOJUY5BX7HQD4OMC2NHCLQ6YHYF5WC,
        body = {
          type = SET_OPTIONS,
          setOptionsOp = {
            inflationDest = NULL,
            clearFlags = NULL,
            setFlags = NULL,
            masterWeight = NULL,
            lowThreshold = 1,
            medThreshold = 1,
            highThreshold = 1,
            homeDomain = NULL,
            signer = NULL
          }
        } }
    ],
    ext = {
      v = 0
    }
  },
  signatures = [
  ]
}

The root of this problem is on the following line

for _, addy := range beforeSortedSigners {
weight, ok := after[addy]
if !ok {
effects.add(source.Address(), history.EffectSignerRemoved, map[string]interface{}{
"public_key": addy,
})
continue
}
effects.add(source.Address(), history.EffectSignerUpdated, map[string]interface{}{
"public_key": addy,
"weight": weight,
})
}

When looking at the before and after state, we add a delete event if the key is not present on the new state; otherwise, we add an update event.

To fix this, we need to add an extra check on the weight, and if it's different, then it is an update; otherwise we should not add an effect.

@abuiles
Copy link
Contributor Author

abuiles commented Feb 6, 2020

From @MonsieurNicolas

@jrice What do you think of removing the duplicate debit/remove effects in an account merge attack (see last 4 effects of https://horizon.stellar.org/accounts/GBSSDVJTXF6XJUN5EWCUXDMCGYGNCIDTPL6G6WM6FTG3G4GDGDKU7Z2T/effects )
The last two debit/remove effects on the already merged account actually never affected the state, so it seems more correct to omit them. Only the account_credit effect on the receiving account is applied twice.
Otherwise, summing all seen amounts in effects of an account could produce negative balances.

@abuiles
Copy link
Contributor Author

abuiles commented Mar 6, 2020

Fixed in #2349

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants