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

simulate fails with empty signature on a rekeyed account #5914

Open
joe-p opened this issue Jan 21, 2024 · 15 comments · May be fixed by #5942
Open

simulate fails with empty signature on a rekeyed account #5914

joe-p opened this issue Jan 21, 2024 · 15 comments · May be fixed by #5942
Assignees
Labels
bug Something isn't working

Comments

@joe-p
Copy link
Contributor

joe-p commented Jan 21, 2024

Subject of the issue

When using simulate with the "allow empty signatures" option, algod will still check the authorizer causing a failure

Your environment

3.21.0.stable [rel/stable] (commit #2c328251)

Steps to reproduce

  1. Attempt to simulate a transaction from an account with a non-zero auth-addr and no signature

Expected behaviour

algod simulates the transaction without a failure

Actual behaviour

faliure message: transaction UOT36H44LYQHOPMLVMVOOBO6EEMRZABHC34XMWPKSY3PGFZWMYOQ: should have been authorized by 3REKUY27LL7OXNA4A63DYMUV766WTRUK5LKSDKAYC2JI7O2GKZR73MK5LA but was actually authorized by P7WXTXBYUVF4AMP6ERLZS6A3JZRMCWCJNJ35ADW5QRZS3F4BL3ZASW47WI

Test Script

import * as algokit from '@algorandfoundation/algokit-utils'

async function main() {
    const algod = new algosdk.Algodv2('a'.repeat(64), 'http://localhost', 4001)
    const kmd = new algosdk.Kmd('a'.repeat(64), 'http://localhost', 4002)

    const sender = algosdk.generateAccount()
    const authAddr = algosdk.generateAccount()

    await algokit.ensureFunded({ accountToFund: sender.addr, minSpendingBalance: algokit.microAlgos(2_000) }, algod, kmd)

    console.log(`Sender: ${sender.addr}, Auth: ${authAddr.addr}`)

    const rekeyTxn = algosdk.makePaymentTxnWithSuggestedParamsFromObject({
        from: sender.addr,
        to: sender.addr,
        amount: 0,
        rekeyTo: authAddr.addr,
        suggestedParams: await algod.getTransactionParams().do()
    })

    const firstEmpySimAtc = new AtomicTransactionComposer()

    const simReq = new algosdk.modelsv2.SimulateRequest({
        txnGroups: [],
        allowUnnamedResources: true,
        allowEmptySignatures: true,
    })

    firstEmpySimAtc.addTransaction({ txn: rekeyTxn, signer: algosdk.makeEmptyTransactionSigner() })

    const preSimAuthAddr = (await algod.accountInformation(sender.addr).do())['auth-addr']
    console.log(`Pre simulate auth addr: ${preSimAuthAddr}`)
    const firstResp = await firstEmpySimAtc.simulate(algod, simReq)

    console.log('failureMessage:', firstResp.simulateResponse.txnGroups[0].failureMessage)

    console.log('Empty signer simulate worked')

    const rekeyAtc = new AtomicTransactionComposer()
    rekeyAtc.addTransaction({ txn: rekeyTxn, signer: algosdk.makeBasicAccountTransactionSigner(sender) })

    const preExecuteAuthAddr = (await algod.accountInformation(sender.addr).do())['auth-addr']
    console.log(`Pre execute auth addr: ${preExecuteAuthAddr}`)
    await rekeyAtc.execute(algod, 3)

    console.log(`${sender.addr} now rekeyed to ${(await algod.accountInformation(sender.addr).do())['auth-addr']}`)

    const secondEmpySimAtc = new AtomicTransactionComposer()

    const rekeyBackTxn = algosdk.makePaymentTxnWithSuggestedParamsFromObject({
        from: sender.addr,
        to: sender.addr,
        amount: 0,
        rekeyTo: sender.addr,
        suggestedParams: await algod.getTransactionParams().do()
    })

    secondEmpySimAtc.addTransaction({ txn: rekeyBackTxn, signer: algosdk.makeEmptyTransactionSigner() })

    const group = await secondEmpySimAtc.gatherSignatures()

    group.forEach((txn) => {
        console.log(algosdk.decodeSignedTransaction(txn).sig)
    })

    const resp = await secondEmpySimAtc.simulate(algod, simReq)

    // failureMessage: transaction UOT36H44LYQHOPMLVMVOOBO6EEMRZABHC34XMWPKSY3PGFZWMYOQ: should have been authorized by 3REKUY27LL7OXNA4A63DYMUV766WTRUK5LKSDKAYC2JI7O2GKZR73MK5LA but was actually authorized by P7WXTXBYUVF4AMP6ERLZS6A3JZRMCWCJNJ35ADW5QRZS3F4BL3ZASW47WI
    console.log('failureMessage:', resp.simulateResponse.txnGroups[0].failureMessage)
}

main()
@joe-p
Copy link
Contributor Author

joe-p commented Jan 21, 2024

I initially closed this because I thought my test script was working, but I just wasn't checking failureMessage. Issue has been updated with a test script that demonstrates the issue

@joe-p joe-p changed the title simulate throws error with rekeyed sender, even with allow empty signatures enabled simulate fails with rekeyed sender with allow empty signatures enabled Jan 22, 2024
@joe-p joe-p changed the title simulate fails with rekeyed sender with allow empty signatures enabled simulate fails with empty signature on a rekeyed account Jan 22, 2024
@jasonpaulos
Copy link
Member

jasonpaulos commented Jan 22, 2024

This is an interesting problem because knowing the rekey status of an account is essential to creating a valid transaction.

As I see it, the benefit of allowing empty signatures is to bypass the need to have/use a private key to sign a transaction for simulation. But there's a step before this: knowing which public/private key pair should be used to sign it in the first place (what we commonly call the auth addr of an account).

In the spirit of simulate being as close to real execution as possible, I find this to be an important thing to get right -- if I have a transaction from Alice authorized by Bob, simulate should reject this if Alice hasn't rekeyed to Bob. You can still simulate this with empty signatures by setting the AuthAddr field properly on a signed transaction object.

I wonder if you'd find this to be an acceptable solution: we augment the SDK "empty signing" functions to take an optional address for the authorizer of the transaction. If this address is provided and different than the sender of the transaction, we would assign it to the AuthAddr field on the signed transaction. I expect simulate will treat the transaction properly then.

@joe-p
Copy link
Contributor Author

joe-p commented Jan 22, 2024

Ah yeah that's a good point. No reason this can't be done client side. The below signer function works well. One "problem" is that this basically requires a call out to algod for every account because the client might not always know the auth addr. This is not a significant problem because if one is using the simulate endpoint they presumably can use the accounts endpoint, but it could mean up to 16 algod calls per group being simulated.

    const makeEmptyTransactionSigner = (authAddr?: string) => {
        return async (txns: algosdk.Transaction[], indexesToSign: number[]) => {
            const emptySigTxns: Uint8Array[] = []

            indexesToSign.forEach(i => {
                const encodedStxn: algosdk.EncodedSignedTransaction = {
                    txn: txns[i].get_obj_for_encoding(),
                };

                if (authAddr) encodedStxn.sgnr = Buffer.from(algosdk.decodeAddress(authAddr).publicKey);

                emptySigTxns.push((algosdk.encodeObj(encodedStxn)));
            });

            return emptySigTxns
        }
    }

@jannotti
Copy link
Contributor

jannotti commented Jan 22, 2024

the client might not always know the auth addr.

I think that @jasonpaulos 's point is that they must! If someone builds some crazy thing that constantly rekeys an account, and it therefore challenging for clients to sign things properly, then that complication should also rear its head during simulation.

Or, perhaps you're sort of saying: "Hey, maybe simulation ought to tell me, the caller, who the current auth-addr is, so I can sign properly". That starts to sound like a thing simulate might do for you. (Though I think we're starting to speculate quite a bit if we think such an app would exist without an obvious example of that sort.)

@joe-p
Copy link
Contributor Author

joe-p commented Jan 22, 2024

I think that @jasonpaulos 's point is that they must!

I agree in the context of using simulate to determine the effects/validity of a specific txn group, but the use case I had in mind was using simulate to get unnamed resources to populate the resource arrays in an app call.

The main problem is that right now wallets do not give clients auth addresses of connected accounts, only the address ie PeraWalletConnect.connect(): Promise<string[]> where string[] is just the address.

@jannotti
Copy link
Contributor

I think I see, the wallet is thinking it's going to do the signing, so what's the point in telling the dapp code how that is going to happen? So you want to be able to tell simulate "I may not know how to sign this, but my wallet will, so just accept it."

One possibility is that this implies a path through the wallet for calling simulate. That sounds like another round of specifications and convincing wallets though.

@joe-p
Copy link
Contributor Author

joe-p commented Jan 22, 2024

I think I see, the wallet is thinking it's going to do the signing, so what's the point in telling the dapp code how that is going to happen? So you want to be able to tell simulate "I may not know how to sign this, but my wallet will, so just accept it."

One possibility is that this implies a path through the wallet for calling simulate. That sounds like another round of specifications and convincing wallets though.

Yeah exactly. Ideally trying to avoid that.

Perhaps there could be an additional query param like "skipAuthAddrCheck" that just overrides the authAddr on the transactions to the correct authorizer automatically?

@jasonpaulos
Copy link
Member

Perhaps there could be an additional query param like "skipAuthAddrCheck" that just overrides the authAddr on the transactions to the correct authorizer automatically?

Without modifying the core eval code (which I'd like to avoid at all cost), that may be a difficult thing to implement. Auth addrs can actually be manipulated arbitrarily at runtime. An app can rekey any account it controls to any other account. Statically you can't figure out what it will do before execution.

So if I have a group of these transactions:

  1. Any type - rekeys sender S to app A
  2. App call to app A - rekeys S to X
  3. Any type - sender is S

It's not necessarily possible to predict what auth addr to give to txn 3, since X could be anything.

But of course if someone wants to actually submit a group like that, they'll have to provide the correct signature for txn 3, so they'll know the right thing to do.

@joe-p
Copy link
Contributor Author

joe-p commented Jan 22, 2024

Ah right didn't think about rekeys within the group. Sounds like the options are

A) Modify the core logic to include logic that skips authAddr check
B) Have clients lookup authAddr when not known
C) Push a new spec that has wallets return authAddr

A should be avoided, so I'm thinking the best solution is B in the short term and C in the long term unless I am not thinking of another option

@jasonpaulos
Copy link
Member

I actually had an idea of how to modify transactions in real time to always know the right auth addr and trick the evaluator. Here's the proof of concept: jasonpaulos@141ab94

So it may be possible to implement without modifying the core eval logic, if we're willing to accept this type of trick.

@joe-p
Copy link
Contributor Author

joe-p commented Jan 23, 2024

Ah smart! So to make sure I'm understanding this, simulate calls have an EvalTracer that implement a callback function executed after program eval, and during that we can check for the new authAddr and update the next txn prior to its eval? I imagine we'd also need to do some static analysis of the rekeyTo fields on no-program calls as well?

@jasonpaulos
Copy link
Member

Yes that's correct. These improvements would have to be made to that proof of concept for the feature to be correct:

  • Prior to simulation eval, lookup each sender to find their current auth addr
  • Scan the top level txns in the group to determine if any rekeys happen and adjust accordingly
  • Since app calls can be few and far between, and an app can rekey multiple accounts, after each top level program, check if the the auth addrs of the senders of the remaining programs change

@joe-p
Copy link
Contributor Author

joe-p commented Jan 24, 2024

Got it. Sounds like something I can take a stab at

@pbennett
Copy link

pbennett commented Apr 6, 2024

Hopefully we can see this fixed soon. Simulate is critical in what I'm building right now and rekeyed accounts are failing right now.

@joe-p
Copy link
Contributor Author

joe-p commented Apr 6, 2024

@pbennett I should be able to get back to it this month. For now you can iterate over all your outer transactions, check the chain for auth-addr and manually set the signer field

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants