Gravita-Protocol / Gravita-SmartContracts

GNU General Public License v3.0
49 stars 31 forks source link

Use Ownable2Step while transfer of ownership #319

Closed pavankv241 closed 1 year ago

pavankv241 commented 1 year ago

*Affected Smart contracts** CommunityIssuance.sol

Impact setAddresses() function use to initiate the contract with setting address . setAdminContract() function use to transfer ownership of admin contract . 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 onlyAdmin()/onlyOwner( ) functions forever, which includes the changing/setting of various critical functions.When noticed, due to a failing auth/onlyOwner( ) function call, it will force the redeployment of the CommunityIssuance.sol contract and require appropriate changes and notifications for switching from the old to new address. This will diminish trust in markets and can incur a significant reputational damage.

Severity Low

code snippet:- https://github.com/Gravita-Protocol/Gravita-SmartContracts/blob/main/contracts/GRVT/CommunityIssuance.sol#L58-L61

Same here https://github.com/Gravita-Protocol/Gravita-SmartContracts/blob/main/contracts/GRVT/GRVTStaking.sol#L166

Reference :- See similar High Risk severity finding from Trail-of-Bits Audit of Hermez: Link to reference

See similar Medium Risk severity finding from Trail-of-Bits Audit of Uniswap V3: Link to reference

Description :-

Recommedation :- 1)Use a two-step address change to new owner address separately using setter functions:

Step 1) Approve a new address as a pendingOwner Step 2) A transaction from the pendingOwner (newOwner) 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.

2)Another recommended alternative is to use Openzeppelin Ownable2Step.sol. Ownable2Step is safer than Ownable for smart contracts because the owner cannot accidentally transfer smart contract ownership to a non-existent address. Rather than directly transferring to the new owner, the transfer only completes when the new owner accepts ownership. Ownable2Step was released in January 2023 during the OpenZeppelin version 4.8 update.

Here are the docs and the code reference links, Ownable2Step docs Ownable2Step code

3)Consider adding a time-delay for such sensitive actions. And at a minimum, use a multisig owner address and not an EOA.

0xfornax commented 1 year ago

We appreciate the submission but this would be a critique of best practices, which is not eligible according to the competition rules.