wrapETH2LD permissioning is over-extended

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L210-L237

Vulnerability details

Impact

Undesired use of ens wrapper

Proof of Concept

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L219-L223

Current permissioning for wrapETH2LD allows msg.senders who are not owner to call it if they are EITHER approved for all on the ERC721 registrar or approved on the wrapper. Allowing users who are approved for the ERC721 registrar makes sense. By giving them approval, you are giving them approval to do what they wish with the token. Any other restrictions are moot regardless because they could use approval to transfer themselves the token anyways and bypass them as the new owner. The issue is allowing users who are approved for the wrapper contract to wrap the underlying domain. By giving approval to the contract the user should only be giving approval for the wrapped domains. As it is currently setup, once a user has given approval on the wrapper contract they have essentially given approval for every domain, wrapped or unwrapped, because any unwrapped domain can be wrapped and taken control of. This is an over-extension of approval which should be limited to the tokens managed by the wrapper contract and not extend to unwrapped domains

Tools Used

Recommended Mitigation Steps

Remove L221

[Arachnid]

This was by design, but the warden raises a good point about the implications of this permission model. Recommend downgrading to QA.

[dmvt]

I'm going to downgrade this to medium. There are not assets at direct risk, but with external factors the assets could be at risk due to the user being unaware that in approving wrapped domains, they are also approving unwrapped domains.