Fix issues for security audit #9
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- remove makeProxyTx and proxy - update comments for AccountFactory - Add custom error for Validator invalid credentials
rishibalakrishnan
force-pushed
the
rishi/security_audit
branch
from
70071c6
to
7a7eab1
Compare
bennetyee
approved these changes
Oct 16, 2024
There was a problem hiding this comment.
this LGTM, though it would be slightly better if this were committed along with the changes for makeProxyTx and proxy functionality -- which is being moved to the sample application -- all together. since there are no dependencies on makeProxyTx etc, having the functionality disappear on the main branch now and reappear shortly is not a big deal.
|
Got it, thought I would have the commit finalized sooner but have been running into some weird issues with local contract development so I will merge this and tag you on the relevant changes on the sample application when they're ready |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I decided to remove makeProxyTx() and proxy() from the KMaaS base functionality. I initially included it because I thought that it would allow the KMaaS account to also function as a gas relayer, but comments made me reconsider. If
proxyis separated into another contract for use on other EVM chains, then 1.makeProxyTxdoesn't work because it doesn't know where to point to and 2. nonce management doesn't make sense because the same keypair can sign transactions on multiple different chains. ThenmakeProxyTxjust replicatessignEIP155functionality with more parameters hard-coded. After trying to approach it a couple different ways I decided it just makes more sense to let developers define how they would want relaying to work.Instead I decided to include that functionality in the sample application instead. The sample application previously didn't rely on gas relayers so required the public keys stored by the KMaaS application to be funded. Instead, I'm in the process of updating it so that user accounts don't have to pay gas to submit transactions.