-
Notifications
You must be signed in to change notification settings - Fork 0
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
QA Report #67
Comments
C4-001 and C4-002 is described in #2 |
C4-001 : Front-runnable InitializersAgree with finding in lack of any mitigation am marking this valid. C4-002 : Initializer reentrancy may lead to double initializationFinding is valid, and mitigation is to upgrade to newer OZ code C4-003 : ERC1155Supply vulnerability in OpenZeppelin ContractsValid C4-004 : Missing zero-address check in the setter functions and initialize functionsAgree e.g. -> Vulnerability in OZ -> See disclosure -> Upgrade to xyz |
Making this the winner of QA Reports, mostly because it offers some unique findings in a proper QA Format. In terms of Findings Kirk-Baird is basically there, I ended up making this report win as it was submitted as QA rather than an aggregate of downgraded findings. That said I believe this report could have had a couple extra findings to make it truly a winner. C4-001: Front-runnable Initializers -> Low C4-002 : Initializer reentrancy may lead to double initialization -> Low C4-003 : ERC1155Supply vulnerability in OpenZeppelin Contracts -> Low C4-004 : Missing zero-address check in the setter functions and initialize functions -> Low |
C4-001 : Front-runnable Initializers
Impact - LOW
All contract initializers were missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.
Proof of Concept
Tools Used
Manual Code Review
Recommended Mitigation Steps
While the code that can be run in contract constructors is limited, setting the owner in the contract's constructor to the
msg.sender
and adding theonlyOwner
modifier to all initializers would be a sufficient level of access control.C4-002 : Initializer reentrancy may lead to double initialization
Impact - LOW
Initializer functions that are invoked separate from contract creation (the most prominent example being minimal proxies) may be reentered if they make an untrusted non-view external call.
Once an initializer has finished running it can never be re-executed. However, an exception put in place to support multiple inheritance made reentrancy possible in the scenario described above, breaking the expectation that there is a single execution.
Note that upgradeable proxies are commonly initialized together with contract creation, where reentrancy is not feasible, so the impact of this issue is believed to be minor.
Proof of Concept
Go to contracts directory.
On the package.json, openzeppelin 4.3.2 is defined.
https://github.com/skalenetwork/ima-c4-audit/blob/main/package.json
Tools Used
None
Recommended Mitigation Steps
Avoid untrusted external calls during initialization.
A fix is included in the version v4.4.1 of @openzeppelin/contracts and @openzeppelin/contracts-upgradeable.
Reference
GHSA-9c22-pwxw-p6hx
C4-003 : ERC1155Supply vulnerability in OpenZeppelin Contracts
Impact - LOW
When ERC1155 tokens are minted, a callback is invoked on the receiver of those tokens, as required by the spec. When including the ERC1155Supply extension, total supply is not updated until after the callback, thus during the callback the reported total supply is lower than the real number of tokens in circulation.
If a system relies on accurately reported supply, an attacker may be able to mint tokens and invoke that system after receiving the token balance but before the supply is updated.
Proof of Concept
Go to contracts directory.
On the package.json, openzeppelin 4.3.2 is defined.
https://github.com/skalenetwork/ima-c4-audit/blob/main/package.json
Tools Used
None
Recommended Mitigation Steps
A fix is included in version 4.3.3 of @openzeppelin/contracts and @openzeppelin/contracts-upgradeable.
Reference
GHSA-wmpv-c2jp-j2xg
C4-004 : Missing zero-address check in the setter functions and initialize functions
Impact - LOW
Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are updated incorrectly.
Proof of Concept
Tools Used
Code Review
Recommended Mitigation Steps
Consider adding zero-address checks in the discussed constructors:
require(newAddr != address(0));.
The text was updated successfully, but these errors were encountered: