raymogg
commented
3 years ago Correct that having the owner be set to a wrong address could be detrimental, however for the first deploy of the factory, this will be owned by the DAO and will be easy to validate on deployment.
Subsequent ownership transfers will be done via DAO proposal, and will have many eyes across them (due to them being a public Tracer DAO proposal) before function execution happens.
For this reason it seems like a lot of overhead to have a two step process for this. Not withstanding that the issue you mention could still be possible
Handle
0xRajeev
Vulnerability details
Impact
The Tracer Perpetuals Factory contract is arguably the most critical contract in the project given that it deploys all the markets. The ownership of this contract is transferred to _governance address, i.e. TracerDAO, in the constructor. This critical address transfer in one-step is very risky because it is irrecoverable from any mistakes.
Impact: If an incorrect address, e.g. for which the private key is not known, is used accidentally then it prevents the use of all the onlyOwner() functions forever, which includes the changing of various deployer contract addresses and market approvals. This use of incorrect address may not even be immediately apparent given that these functions are probably not used immediately. When noticed, due to a failing onlyOwner() function call, it will force the redeployment of the factory contract and require appropriate changes and notifications for switching from the old to new address. This will diminish trust in markets and incur a significant reputational damage.
Proof of Concept
See similar High Risk severity finding from Trail-of-Bits Audit of Hermez: https://github.com/trailofbits/publications/blob/master/reviews/hermez.pdf
See similar Medium Risk severity finding from Trail-of-Bits Audit of Uniswap V3: https://github.com/Uniswap/uniswap-v3-core/blob/main/audits/tob/audit.pdf
https://github.com/code-423n4/2021-06-tracer/blob/74e720ee100fd027c592ea44f272231ad4dfa2ab/src/contracts/TracerPerpetualsFactory.sol#L43
https://github.com/code-423n4/2021-06-tracer/blob/74e720ee100fd027c592ea44f272231ad4dfa2ab/src/contracts/TracerPerpetualsFactory.sol#L68
https://github.com/code-423n4/2021-06-tracer/blob/74e720ee100fd027c592ea44f272231ad4dfa2ab/src/contracts/TracerPerpetualsFactory.sol#L119
https://github.com/code-423n4/2021-06-tracer/blob/74e720ee100fd027c592ea44f272231ad4dfa2ab/src/contracts/TracerPerpetualsFactory.sol#L124
https://github.com/code-423n4/2021-06-tracer/blob/74e720ee100fd027c592ea44f272231ad4dfa2ab/src/contracts/TracerPerpetualsFactory.sol#L129
https://github.com/code-423n4/2021-06-tracer/blob/74e720ee100fd027c592ea44f272231ad4dfa2ab/src/contracts/TracerPerpetualsFactory.sol#L134
https://github.com/code-423n4/2021-06-tracer/blob/74e720ee100fd027c592ea44f272231ad4dfa2ab/src/contracts/TracerPerpetualsFactory.sol#L144
Tools Used
Manual Analysis
Recommended Mitigation Steps
Retain the deployer ownership in the constructor and then use a two-step address change to _governance address separately using setter functions: 1) Approve a new address as a pendingOwner 2) A transaction from the pendingOwner (TracerDAO) address claims the pending ownership change. This mitigates risk because if an incorrect address is used in step (1) then it can be fixed by re-approving the correct address. Only after a correct address is used in step (1) can step (2) happen and complete the address/ownership change.