[Improvement] Align renounce logic between ERC173Facet and LibERC173

[CodeCryptX]

Description

ERC173Facet and LibERC173 handle ownership renounce conditions differently.

• LibERC173.transferOwnership reverts if the owner is already renounced (address(0)).
• ERC173Facet.transferOwnership does not perform this check.

Expected Behavior

Both should revert under the same conditions to maintain consistent behavior across the diamond.

Suggested Fix

Add the renounce check in ERC173Facet.transferOwnership:

if (s.owner == address(0)) revert OwnableAlreadyRenounced();

[maxnorm]

Did you already made the update? I'm going to make it, if not

[CodeCryptX]

No i didn't.

…

On Mon, 20 Oct 2025, 22:22 M.N, ***@***.***> wrote: *maxnorm* left a comment (Perfect-Abstractions/Compose#46) <#46 (comment)> Did you already made the update? I'm going to make it, if not — Reply to this email directly, view it on GitHub <#46 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BI4N4QWH3CAMTWM6J74NG3D3YUK4NAVCNFSM6AAAAACJWAVEKCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTIMRTGA2DSNZWGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[maxnorm]

I suggest doing the inverse.

Suggested Fix

Change the if statement in LibERC173.transferOwnership to:
if (msg.sender != s.owner) revert OwnableUnauthorizedAccount();

Rational

Doing s.owner == address(0), would make a redundant check since msg.sender != s.owner is necessary to avoid unauth msg.sender and if ownership is already renounced, s.owner = 0, the msg.sender will never equal to 0.

@mudgen Can you share the rational between these diffs between the facet & lib? I might also be wrong on this depending on the overall pattern goal with library

Edit: i'm probably wrong because lib are only internal logic only. this would be to by highlight that using the lib standalone doesn't validate the msg.sender

[mudgen]

@maxnorm I don't have an opinion on this logic other than let's do what makes the most sense. And let's do it the same way that OpenZepplin does it to maintain compatibility unless the way OpenZeppelin does it doesn't make sense.

[maxnorm]

@maxnorm I don't have an opinion on this logic other than let's do what makes the most sense. And let's do it the same way that OpenZepplin does it to maintain compatibility unless the way OpenZeppelin does it doesn't make sense.

OZ is using the onlyOwner modifier with this check: owner() != _msgSender() inside transferOwnership and renounceOwnership. Which it's the same as msg.sender != s.owner.

My question would be more on who's the msg.sender? I'm not fully familiar with diamond but as i understand it's the diamond since it's delegating the call to the facet.

I also notice while comparing with OZ impl of Ownable. transferOwnership doesn't check if the _newOwner is not the address(0). I can open a new issue on this for after the renounceOwnership PR if we consider that ownership transfer can't be to address(0)

[mudgen]

@maxnorm Whoever or whatever calls a function on a diamond contract is msg.sender. msg.sender does not change when a diamond executes code from a facet.

[maxnorm]

Perfect.

In this case, making the change to if (msg.sender != s.owner) make sense in the lib and the facets