fix(proxy): add zero address validation for proxy parameter #6257
Conversation
|
WalkthroughThis pull request modifies the ProxyAdmin contract to add input validation for the proxy parameter. A new error type Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/proxy/transparent/ProxyAdmin.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Redirect rules - solidity-contracts
- GitHub Check: halmos
- GitHub Check: slither
- GitHub Check: tests
- GitHub Check: tests-upgradeable
- GitHub Check: tests-foundry
- GitHub Check: coverage
- GitHub Check: Header rules - solidity-contracts
- GitHub Check: Pages changed - solidity-contracts
🔇 Additional comments (2)
contracts/proxy/transparent/ProxyAdmin.sol (2)
14-17: LGTM! Well-defined custom error.The error definition follows OpenZeppelin conventions and provides clear semantics. Including the
proxyaddress parameter aids debugging.
40-40: LGTM! Clear requirement documentation.The documentation accurately reflects the new validation requirement.
| if (address(proxy) == address(0)) { | ||
| revert ProxyAdminInvalidProxy(address(0)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -e test.js -e test.ts -e spec.js -e spec.ts | grep -i proxy | head -20Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 447
🏁 Script executed:
rg "ProxyAdminInvalidProxy" --type=js --type=ts -lRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 61
🏁 Script executed:
rg "upgradeAndCall" --type=js --type=ts -l | grep testRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 120
🏁 Script executed:
cat -n test/proxy/transparent/ProxyAdmin.test.js | head -150Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 3882
🏁 Script executed:
rg "upgradeAndCall|ProxyAdminInvalidProxy" test/proxy/transparent/ProxyAdmin.test.js -B 2 -A 5Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 1802
🏁 Script executed:
cat -n contracts/proxy/transparent/ProxyAdmin.sol | sed -n '40,60p'Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 707
🏁 Script executed:
rg "ProxyAdminInvalidProxy" contracts/proxy/transparent/ProxyAdmin.sol -B 2 -A 2Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 433
Add test coverage for zero address validation.
The defensive check is well-implemented, but the test file lacks coverage for the ProxyAdminInvalidProxy error when a zero address is passed. Add a test case to verify that upgradeAndCall reverts with ProxyAdminInvalidProxy when the proxy address is address(0).
🤖 Prompt for AI Agents
In contracts/proxy/transparent/ProxyAdmin.sol around lines 49-51, add a unit
test that calls upgradeAndCall with proxy address set to address(0) and expects
it to revert with the custom error ProxyAdminInvalidProxy; create or update the
ProxyAdmin test (e.g., test/ProxyAdmin.test.ts) to invoke
proxyAdmin.upgradeAndCall(ethers.constants.AddressZero, implAddress, callData)
and assert using Chai/Hardhat helpers: await
expect(<call>).to.be.revertedWithCustomError(proxyAdmin,
"ProxyAdminInvalidProxy").withArgs(ethers.constants.AddressZero), ensuring the
test supplies any needed mocks/fixtures for proxyAdmin and implementation
addresses.
2 participants
Description
Adds explicit zero address validation for the proxy parameter in ProxyAdmin.upgradeAndCall() to prevent low-level call failures and provide clearer error messages.
Changes