-
Notifications
You must be signed in to change notification settings - Fork 8
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
Destruction of the SmartAccount
implementation
#496
Labels
3 (High Risk)
Assets can be stolen/lost/compromised directly
bug
Something isn't working
H-01
primary issue
Highest quality submission among a set of duplicates
satisfactory
satisfies C4 submission criteria; eligible for awards
selected for report
This submission will be included/highlighted in the audit report
sponsor confirmed
Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Comments
code423n4
added
3 (High Risk)
Assets can be stolen/lost/compromised directly
bug
Something isn't working
labels
Jan 9, 2023
c4-judge
added
the
primary issue
Highest quality submission among a set of duplicates
label
Jan 17, 2023
gzeon-c4 marked the issue as primary issue |
This was referenced Jan 17, 2023
c4-judge
added
the
satisfactory
satisfies C4 submission criteria; eligible for awards
label
Jan 17, 2023
gzeon-c4 marked the issue as satisfactory |
This was referenced Jan 17, 2023
#14 also note that if owner is left to address(0) some validation can be bypassed |
This was referenced Jan 17, 2023
livingrockrises marked the issue as sponsor confirmed |
c4-sponsor
added
the
sponsor confirmed
Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
label
Jan 25, 2023
#6 is not duplicate of this issue |
#43 is also not duplicate of this issue |
gzeon-c4 marked the issue as selected for report |
c4-judge
added
the
selected for report
This submission will be included/highlighted in the audit report
label
Feb 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
3 (High Risk)
Assets can be stolen/lost/compromised directly
bug
Something isn't working
H-01
primary issue
Highest quality submission among a set of duplicates
satisfactory
satisfies C4 submission criteria; eligible for awards
selected for report
This submission will be included/highlighted in the audit report
sponsor confirmed
Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Lines of code
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L166
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L192
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L229
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/base/Executor.sol#L23
Vulnerability details
Description
If the
SmartAccount
implementation contract is not initialized, it can be destroyed using the following attack scenario:SmartAccount
implementation contract using theinit
function.delegatecall
to a contract that executes theselfdestruct
opcode on any incoming call, such as:The destruction of the implementation contract would result in the freezing of all functionality of the wallets that point to such an implementation. It would also be impossible to change the implementation address, as the
Singleton
functionality and the entire contract would be destroyed, leaving only the functionality from the Proxy contract accessible.In the deploy script there is the following logic:
So, in the deploy script there is no enforce that the
SmartAccount
contract implementation was initialized.The same situation in
scw-contracts/scripts/wallet-factory.deploy.ts
script.Please note, that in case only the possibility of initialization of the
SmartAccount
implementation will be banned it will be possible to use this attack. This is so because in such a caseowner
variable will be equal to zero and it will be easy to pass a check inside ofcheckSignatures
function using the fact that for incorrect input parametersecrecover
returns a zero address.Impact
Complete freezing of all functionality of all wallets (including complete funds freezing).
Recommended Mitigation Steps
Add to the deploy script initialization of the
SmartAccount
implementation, or add to theSmartAccount
contract the following constructor that will prevent implementation contract from the initialization:The text was updated successfully, but these errors were encountered: