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 panic in ExitToNear #865

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

guidovranken
Copy link
Contributor

Description

If input is empty a panic occurs. This PR sets flag to the default value in that case.

Performance / NEAR gas cost considerations

Testing

Fuzzing.

How should this be reviewed

Ensure that flag assuming the default value if input is empty is the intended behavior.

Additional information

Copy link
Member

@birchmd birchmd left a comment

Choose a reason for hiding this comment

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

Good catch, thanks!

If the input is empty it will fail the validate_input_size check in parse_input immediately after the assignment of flag, so the default value will never be used, which I think is a good thing. Instead of assigning a default value, we could potentially move flag assignment input parse_input after the size is already validated. But maybe that's more complexity than necessary.

@aleksuss
Copy link
Member

Yes. But the only problem is that the parse_input returns modified input. I would propose modifying the parse_input function so it will return the flag as well:

#[cfg(feature = "error_refund")]
let (flag, refund_address, mut input) = parse_input(input)?;
#[cfg(not(feature = "error_refund"))]
let (flag, mut input) = parse_input(input)?;

@birchmd
Copy link
Member

birchmd commented Nov 14, 2023

@aleksuss Yes, that is what I suggested above. My worry about complexity is because of the conditional compilation you would need to change both versions of the parse_input function (duplicating the flag assignment logic). It's not a big deal to duplicate that one line, and I agree it's nicer to encapsulate all the input handling into one function (even if there are two different versions of that function). So I think I agree with you that it's the better solution overall.

@aleksuss
Copy link
Member

On the other hand, even if we get the default value for the flag, we will fail later because we don't pass the validation. So, I guess it's absolutely fine and could be merged.

@aleksuss aleksuss added this pull request to the merge queue Nov 14, 2023
Merged via the queue into aurora-is-near:develop with commit 1e327df Nov 14, 2023
22 checks passed
aleksuss pushed a commit that referenced this pull request Nov 28, 2023
## Description

If `input` is empty a panic occurs. This PR sets `flag` to the default
value in that case.

## Performance / NEAR gas cost considerations

## Testing

Fuzzing.

## How should this be reviewed

Ensure that `flag` assuming the default value if `input` is empty is the
intended behavior.

## Additional information
@aleksuss aleksuss mentioned this pull request Nov 28, 2023
aleksuss added a commit that referenced this pull request Nov 28, 2023
## [3.4.0] 2023-11-28

### Additions

- Added a possibility to pass initialize arguments in json format to the
`new` transaction by [@aleksuss]. ([#871])
- The `SubmitResult` was made available for `ft_on_transfer`
transactions in the standalone engine by [@birchmd]. ([#869])
- The order of producing the exit precompile and XCC promises has been
changed to sequential by [@birchmd]. ([#868])

### Changes

- Removed the code hidden behind the feature that isn't used anymore by
[@joshuajbouw]. ([#870])
- The logic of unwrapping wNEAR has been changed to the Bridge's native
by [@birchmd]. ([#867])
- Bumped the `near-workspaces` to 0.9 by [@aleksuss]. ([#862])

### Fixes

- Add a method for upgrading XCC router contract by [@birchmd]. ([#866])
- Fixed a potential panic in the `ExitToNear` precompile by
[@guidovranken]. ([#865])
- Fixed a behaviour when the `ft_transfer` could occur before the
`near_withdraw` by [@birchmd]. ([#864])
- Fixed correctness of reproducing the NEAR runtime random value in the
standalone engine by [@birchmd]. ([#863])

[#862]: #862
[#863]: #863
[#864]: #864
[#865]: #865
[#866]: #866
[#867]: #867
[#868]: #868
[#869]: #869
[#870]: #870
[#871]: #871

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Michael Birch <michael.birch@aurora.dev>
Co-authored-by: Guido Vranken <guidovranken@users.noreply.github.com>
Co-authored-by: Joshua J. Bouw <joshua@aurora.dev>
aleksuss added a commit that referenced this pull request Nov 28, 2023
## [3.4.0] 2023-11-28

### Additions

- Added a possibility to pass initialize arguments in json format to the
`new` transaction by [@aleksuss]. ([#871])
- The `SubmitResult` was made available for `ft_on_transfer`
transactions in the standalone engine by [@birchmd]. ([#869])
- The order of producing the exit precompile and XCC promises has been
changed to sequential by [@birchmd]. ([#868])

### Changes

- Removed the code hidden behind the feature that isn't used anymore by
[@joshuajbouw]. ([#870])
- The logic of unwrapping wNEAR has been changed to the Bridge's native
by [@birchmd]. ([#867])
- Bumped the `near-workspaces` to 0.9 by [@aleksuss]. ([#862])

### Fixes

- Add a method for upgrading XCC router contract by [@birchmd]. ([#866])
- Fixed a potential panic in the `ExitToNear` precompile by
[@guidovranken]. ([#865])
- Fixed a behaviour when the `ft_transfer` could occur before the
`near_withdraw` by [@birchmd]. ([#864])
- Fixed correctness of reproducing the NEAR runtime random value in the
standalone engine by [@birchmd]. ([#863])

[#862]: #862
[#863]: #863
[#864]: #864
[#865]: #865
[#866]: #866
[#867]: #867
[#868]: #868
[#869]: #869
[#870]: #870
[#871]: #871

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Michael Birch <michael.birch@aurora.dev>
Co-authored-by: Guido Vranken <guidovranken@users.noreply.github.com>
Co-authored-by: Joshua J. Bouw <joshua@aurora.dev>
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.

None yet

3 participants