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

Only allow access to txn effects for previous transactions #3529

Merged

Conversation

jannotti
Copy link
Contributor

txn opcodes should not be able to look at the current or future "effects" fields

@jannotti jannotti requested a review from jasonpaulos January 28, 2022 19:41
@jannotti jannotti self-assigned this Jan 28, 2022

itxn := &cx.Txn.EvalDelta.InnerTxns[len(cx.Txn.EvalDelta.InnerTxns)-1]
sv, err := cx.txnFieldToStack(itxn, fs, arrayFieldIdx, 0, true)
sv, err := cx.opTxnImpl(0, srcGroup, field, ai, true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be srcInner?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes!

Comment on lines 2272 to 2274
const srcGroup txnSource = 0
const srcInner txnSource = 1
const srcInnerGroup txnSource = 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const srcGroup txnSource = 0
const srcInner txnSource = 1
const srcInnerGroup txnSource = 2
const (
srcGroup txnSource = 0
srcInner = 1
srcInnerGroup = 2
)

nit: this would be more recognizable as an enum like this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll even go full on iota on it

tx := &group[gi]

// int(gi) is safe because gi < len(group). Slices in Go cannot exceed `int`
sv, err = cx.txnFieldToStack(tx, fs, ai, int(gi), src != srcGroup)
if err != nil {
cx.err = err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: there's inconsistent use of cx.err. This check sets it, but all other checks in this function don't

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. oddly enough that ends up having the exact same effect, but was certainly not intended

Next up, eliminate this cx.err assignment thing in all the opcodes.

@jannotti jannotti merged commit 84b52e7 into algorand:feature/contract-to-contract Jan 28, 2022
@jannotti jannotti deleted the past-effects-only branch January 28, 2022 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants