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

TxSigLimit checking logic bug #4394

Closed
4 tasks
Hyung-bharvest opened this issue May 22, 2019 · 3 comments · Fixed by #4396
Closed
4 tasks

TxSigLimit checking logic bug #4394

Hyung-bharvest opened this issue May 22, 2019 · 3 comments · Fixed by #4396
Assignees
Labels

Comments

@Hyung-bharvest
Copy link
Contributor

Hyung-bharvest commented May 22, 2019

Summary of Bug

when checking the limit of signature number, it does not use the parameter from genesis.json but it use default value.

Steps to Reproduce

if uint64(sigCount) > DefaultTxSigLimit {
return sdk.ErrTooManySignatures(
fmt.Sprintf("signatures: %d, limit: %d", sigCount, DefaultTxSigLimit),

at line 62&64, DefaultTxSigLimit should be replaced by TxSigLimit.

additionally, gaiacli should check whether the number of signature is approvable in the context of current params in genesis when user creates a multisig account. Because, if not, multisig generation with more than limit number of signature will succeed without any error, so that user can misunderstand that the multisig account is successfully created so he/she can send atoms to the account. Unfortunately user never be able to control the multisig account because of TxSigLimit, causing losing the atoms forever.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

wow...good catch @dlguddus. Unfortunately, we'll have to move this check out of ValidateBasic and into the ante-handler because we need access to the params. Not a big deal I suppose.

With regards to multisig creation/generation, it might be a bit difficult to do that. The gaia CLI does not have access to genesis. Even if it did, it would not matter because the param could change through governance. At best, I think we should just improve the documentation to state that you should not create more signatures than the parameter allows. It will be up to the user to ensure this.

@Hyung-bharvest
Copy link
Contributor Author

Thanks for fixing this ! For gaiacli, yes I think you are right because creating multisig happens offline.

@alexanderbez
Copy link
Contributor

Correct :)

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 a pull request may close this issue.

2 participants