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

Missing lock in tx.go #207

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

mabdullah22
Copy link

There is a missing lock in tx.go , after the transaction is done via the RPC , the wallet is not locked hence its kept open to be used without password. If the RPC is exposed an attacker can make tx request without password as the wallet is unlocked. It is recommended to put defer lock , so after the tx is executed wallet is locked again.

There is a missing lock in tx.go , after the transaction is done via the RPC , the wallet is not locked hence its kept open to be used without password. If the RPC is exposed an attacker can make tx request without password as the wallet is unlocked. It is recommended to put defer lock , after the tx is executed.
@jieyilong
Copy link
Member

Thanks for bring this up, but we also have the thetacli.UnlockKey and thetacli.LockKey API available. The recommended flow is to call thetacli.UnlockKey first to unlock the wallet, send out the tx, and then call thetacli.LockKey immediately to lock the wallet.

@mabdullah22
Copy link
Author

mabdullah22 commented Sep 21, 2023

This is true. But what if a user doesn't lock the wallet , in that case it will be open to attack. As a best practice and more cautious approach we should lock the wallet immediately. For example an attacker can keep bombarding transfer request on the RPC port , as the wallet will be unlock by node user that transaction will be successful.

@mabdullah22
Copy link
Author

What are your thought on above scenario ?

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.

2 participants