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

Constrain token payment by auth entries #31

Merged
merged 7 commits into from
Feb 8, 2024

Conversation

aristidesstaffieri
Copy link
Contributor

@aristidesstaffieri aristidesstaffieri commented Feb 1, 2024

Adds checks during token simulation to attempt to constrain simulation to a typical transfer implementation.

…ctId mismatch, and sub invocations to token payment simulation
@aristidesstaffieri aristidesstaffieri self-assigned this Feb 1, 2024
@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

src/route/index.ts Outdated Show resolved Hide resolved
@aristidesstaffieri aristidesstaffieri changed the title [WIP] Constrain token payment by auth entries Constrain token payment by auth entries Feb 1, 2024
@aristidesstaffieri aristidesstaffieri marked this pull request as ready for review February 1, 2024 19:45
@stellar-jenkins
Copy link

1 similar comment
@stellar-jenkins
Copy link

export const ERROR = {
ACCOUNT_NOT_SOURCE:
"Transfer contains authorization entry for a different account",
FOREIGN_CONTRACT_AUTH:
Copy link

Choose a reason for hiding this comment

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

did we mean to use this error in this pull request or are you planning to use it in a follow-up change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I'll remove that one, @sisuresh pointed how that check was already covered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed in 3272c3c


const built = preparedTransaction.build();
const sorobanOp = built
.operations[0] as Operation.InvokeHostFunction;
Copy link

Choose a reason for hiding this comment

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

should we inspect all operations or is only looking at the first one always sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the case of a tx with an InvokeHostFunction, there can only be 1 operation. We could check for the operation type here but only the InvokeHostFunction type has auth. I'll add an explicit check for the op type though, that seems a bit more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an explicit check for op type in 3272c3c

…or from mercury, removes unused token sim error
@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

@aristidesstaffieri aristidesstaffieri merged commit 4ffc981 into main Feb 8, 2024
4 checks passed
@aristidesstaffieri aristidesstaffieri deleted the constrain-token-payment branch February 8, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request: Better Scam / Exploitation protection for malicious Soroban invocations
4 participants