Skip to content

Return early in PosBase::transfer if an attempt is made to transfer zero tokens#856

Merged
juped merged 4 commits intomainfrom
james/mainline/amount-is-zero
Apr 13, 2023
Merged

Return early in PosBase::transfer if an attempt is made to transfer zero tokens#856
juped merged 4 commits intomainfrom
james/mainline/amount-is-zero

Conversation

@james-chf
Copy link
Copy Markdown
Contributor

Relates to #603

This potential PR would add a helper fn for checking if an Amount::is_zero, and use it to short circuit PosBase::transfer if somehow a zero amount was attempted to be transferred.

@james-chf james-chf force-pushed the james/mainline/amount-is-zero branch from ac057b1 to 508799a Compare December 7, 2022 13:58
@james-chf james-chf force-pushed the james/mainline/amount-is-zero branch from 508799a to be02b10 Compare December 16, 2022 13:36
@james-chf james-chf marked this pull request as ready for review January 10, 2023 09:52
@james-chf
Copy link
Copy Markdown
Contributor Author

pls update wasm

@james-chf
Copy link
Copy Markdown
Contributor Author

james-chf commented Jan 10, 2023

@tzemanovic not sure if this is worth adding, to resolve audit issue #603, if not this PR can be close

@juped juped added this to the 0.14 milestone Jan 10, 2023
@tzemanovic
Copy link
Copy Markdown
Collaborator

@tzemanovic not sure if this is worth adding, to resolve audit issue #603, if not this PR can be close

If I understand the issue correctly, it is that we're not checking that slash amount is greater than 0, which is probably a good thing to verify with cubic slashing. I think we should somehow ensure this from slashing logic, maybe having some lower bound param or something.

The transfer with 0 is almost no-op (it might write 0 into target balance, if there was none before), but I think that might be besides the point. I'm happy to merge this as the early return is better, but I don't think it solves the issue (if I grokked it right). Let's please the leave the issue open for now as we still have to change the slashing logic substantially.

@james-chf
Copy link
Copy Markdown
Contributor Author

james-chf commented Jan 12, 2023

If I understand the issue correctly, it is that we're not checking that slash amount is greater than 0

yep, it is almost a noop like you say, if we were to slash a validator by 0 (which should not really be occurring in any case) - we may end up initializing a balance to be explicitly 0 where there was none before (but no balance and 0 balance should be being treated functionally equivalently)

tzemanovic
tzemanovic previously approved these changes Jan 12, 2023
@tzemanovic tzemanovic modified the milestones: 0.14, 0.15 Feb 8, 2023
@tzemanovic tzemanovic force-pushed the james/mainline/amount-is-zero branch from be02b10 to b5e6d47 Compare March 30, 2023 14:33
@tzemanovic
Copy link
Copy Markdown
Collaborator

I rebased this into v0.14.3 - the PosBase trait is gone and we're just using token::transfer in PoS now, so I moved the short-circuiting there

@tzemanovic
Copy link
Copy Markdown
Collaborator

pls update wasm

tzemanovic added a commit that referenced this pull request Mar 31, 2023
* james/mainline/amount-is-zero:
  [ci] wasm checksums update
  Add changelog
  Short circuit token::transfer if amount is zero
  Add Amount::is_zero fn
@tzemanovic tzemanovic mentioned this pull request Mar 31, 2023
@juped juped merged commit 4de6ded into main Apr 13, 2023
@juped juped deleted the james/mainline/amount-is-zero branch April 13, 2023 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants