Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

Ensure balance accounting is correct when sending multiple transfers with MsgSend #314

Closed
fedekunze opened this issue May 22, 2020 · 2 comments · Fixed by #661
Closed

Ensure balance accounting is correct when sending multiple transfers with MsgSend #314

fedekunze opened this issue May 22, 2020 · 2 comments · Fixed by #661
Assignees

Comments

@fedekunze
Copy link
Contributor

fedekunze commented May 22, 2020

Acceptance Criteria: try to reproduce bug to see if it is still relevant. Fix bug if still relevant.

This bug (I haven't yet reproduced it) can be exploited by using MsgSend which updates the balance in store of a recipient and the fact that the evm module stateObjects are committed to state only during EndBlock.

Logic of the potential vulnerability:

If you send a tx containing: []sdk.Msg{MsgEthereumTx, MsgSend, MsgEthermint}
initialBalance = 100
each msg sends 10 to the same recipient

  • MsgEthereumTx will set the state object with the updated balance (which is not in store yet) (i.e balance in store 100, balance in state object 110 )
  • MsgSend will set the original balance + the new funds to store (i.e balance in store 110, balance in state object 110 )
  • MsgEthereumTx will set the state object with the updated balance (which is not in store yet) (i.e balance in store 110, balance in state object 120 )
  • Finally, EndBlock will set the balance to 120 because that’s what was stored in the state object.

Mitigation options:

  1. Set the objects to state at the moment of the state transition (not during EndBlock)
  2. Fetch the latest balance before setting the new one.
@fedekunze fedekunze added the bug label May 22, 2020
@fedekunze fedekunze self-assigned this May 22, 2020
@fedekunze
Copy link
Contributor Author

same applies to account sequences

@wade-liwei
Copy link

why not directly R/W from bank module ?

// maps that hold 'live' objects, which will get modified while processing a
// state transition
stateObjects      map[ethcmn.Address]*stateObject
stateObjectsDirty map[ethcmn.Address]struct{}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants