search

hats-finance / Smooth-0x64bc275b37e62eec81a00ecaecd2b9567058f990

Dappnode's MEV Smoothing Pool
0 stars 2 forks source link

Possible to renounce ownership #10

Open hats-bug-reporter[bot] opened 1 year ago

hats-bug-reporter[bot] commented 1 year ago

Github username: @0xfuje Submission hash (on-chain): 0x1532c351c658e2dba37e7d192a6ac6b5177f60f95b51e7ee246ffcd030da57fe Severity: low

Description:

Description

DappNodeSmoothingPool uses openzeppelin's OwnableUpgradeable helper contract. The owner of the contract will be the deployer, if the deployer would renounce his ownership: privileged functions protected by onlyOwner() such as initSmoothingPool(), updatePoolFee(), updatePoolFeeRecipient(), updateCheckpointSlotSize() and updateCollateral()couldn't be called anymore because the contract would have no owner.

openzeppelin/contracts-upgradable/access/OwnableUpgradeable.sol

function renounceOwnership() public virtual onlyOwner {
    _transferOwnership(address(0));
}

Recommended Mitigation

It's best practice to not allow the owner to renounce their ownership via renounceOwnership(). Consider to overwrite the function in DappNodeSmoothingPool.sol to revert it.

function  renounceOwnership() public override onlyOwner {
    revert("Can't renounce ownership");
}
invocamanman commented 1 year ago

Duplicated: https://github.com/hats-finance/Smooth-0x64bc275b37e62eec81a00ecaecd2b9567058f990/issues/23

0xfuje commented 1 year ago

Hey @invocamanman, please can you elaborate what do you mean by duplicated? Since my submission was the first one soon after the contest started. Screenshot from 2023-11-06 19-55-37

Thank you!

0xfuje commented 1 year ago

Similar duplication on #9

invocamanman commented 1 year ago

Actually you are right, sorry, the duplicated tag was a mistake. Even tho i consider the issue invalid due the following: This is a contemplated and possible scenario, which meant that the owner won't be necessary anymore. The owner is aim to tweak some parameters of the smoothing pool, and could arrive a point where is not needed anymore. Also i don't think it's worth to implement a custom lib, just for this function, anyway the owner can be "renounced" transferring it to a non EOA that is not able to call this contract. So since it does not add any security, this allows us to use a standard library, and could be a possible use case in the future, so i think it's correct.