Skip to content

[rosetta] reject negative deposit amount in extract_transfer#20238

Open
madib06ops wants to merge 1 commit into
aptos-labs:mainfrom
madib06ops:rosetta-negative-deposit-check
Open

[rosetta] reject negative deposit amount in extract_transfer#20238
madib06ops wants to merge 1 commit into
aptos-labs:mainfrom
madib06ops:rosetta-negative-deposit-check

Conversation

@madib06ops

@madib06ops madib06ops commented Jul 18, 2026

Copy link
Copy Markdown

Description

Transfer::extract_transfer parses the withdraw and deposit operation amounts as i128, then converts the deposit to u64 for the transfer payload. It verifies the two amounts negate each other and that the deposit is not above u64::MAX, but never checks the lower bound, and -withdraw_value != deposit_value holds for either sign orientation. A construction request carrying withdraw.amount.value = "5" and deposit.amount.value = "-5" passes both guards, and deposit_value as u64 wraps to 18446744073709551611.

The amount flows into aptos_stdlib::transfer through InternalOperation::payload, so the unsigned transaction returned by /construction/payloads does not match the operations that were submitted, and nothing downstream re-derives it from those operations. Swapping the one-sided comparison for u64::try_from covers both bounds in the conversion that actually performs the narrowing.

How Has This Been Tested?

Added test_extract_transfer_rejects_negative_deposit in crates/aptos-rosetta/src/test/mod.rs, which builds the sign-flipped operation pair and asserts the extraction is rejected.

Before the change:

test test::test_extract_transfer_rejects_negative_deposit ... FAILED
panicked at crates/aptos-rosetta/src/test/mod.rs:1078:5:
negative deposit amount must be rejected, got Ok(18446744073709551611)

After, cargo test -p aptos-rosetta reports 22 passed, 0 failed, so the existing transfer coverage is unchanged.

Key Areas to Review

Whether the merged error message is acceptable. The over-u64::MAX and negative cases now share one InvalidTransferOperations string instead of the previous upper-bound-only wording; happy-path amounts convert exactly as before.

Type of Change

  • Bug fix

Which Components or Systems Does This Change Impact?

  • Full Node (API, Indexer, etc.)

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code

Note

Medium Risk
Touches Rosetta construction transfer validation; incorrect handling could still break /construction/payloads, but the change is a narrow bounds check with a targeted test.

Overview
Fixes a Rosetta construction bug where sign-flipped withdraw/deposit operations could pass validation and produce a bogus transfer amount.

Transfer::extract_transfer still requires withdraw and deposit to negate each other, but it now narrows the deposit with u64::try_from instead of an upper-bound check plus as u64. Negative deposits (and values above u64::MAX) return InvalidTransferOperations instead of wrapping into a huge unsigned amount that would flow into /construction/payloads via InternalOperation::payload.

Adds test_extract_transfer_rejects_negative_deposit for withdraw 5 / deposit -5.

Reviewed by Cursor Bugbot for commit d0f3d14. Bugbot is set up for automated code reviews on this repo. Configure here.

@madib06ops
madib06ops requested a review from a team as a code owner July 18, 2026 06:58
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.

1 participant