search

code-423n4 / 2022-08-mimo-findings

0 stars 0 forks source link

Registry.sol works bad - it fails to delivere expected functionality #78

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-mimo/blob/eb1a5016b69f72bc1e4fd3600a65e908bd228f13/contracts/proxy/MIMOProxyFactory.sol#L40-L58 https://github.com/code-423n4/2022-08-mimo/blob/eb1a5016b69f72bc1e4fd3600a65e908bd228f13/contracts/proxy/MIMOProxyRegistry.sol#L39-L59

Vulnerability details

Impact

The description of Registry.sol is following: /// Deploys new proxies via the factory and keeps a registry of owners to proxies. Owners can only /// have one proxy at a time. But it is not. There are multiple problems: 1) Proxy owner can change and will not be registered 2) There many ways for an owner to have many proxies:

Proof of Concept

https://github.com/code-423n4/2022-08-mimo/blob/eb1a5016b69f72bc1e4fd3600a65e908bd228f13/contracts/proxy/MIMOProxyFactory.sol#L40-L58 https://github.com/code-423n4/2022-08-mimo/blob/eb1a5016b69f72bc1e4fd3600a65e908bd228f13/contracts/proxy/MIMOProxyRegistry.sol#L39-L59

Tools Used

Hardhat

Recommended Mitigation Steps

Delete Proxy.transfetOwnership() Disallow anyone to call deploy() and deployFor() in Factory()

RnkSngh commented 2 years ago

We agree that this is an issue and intend to fix this

gzeoneth commented 2 years ago

I believe this is High Risk due to the unexpected ownership behavior

m19 commented 2 years ago

While the Registry indeed does not work as advertised I am not sure if high risk is the correct here? As per the definition "Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals)." I don't think that applies here. We also see no way Proxy owner can change and will not be registered actually can happen which would be the only scenario there is a loss of funds.

gzeoneth commented 2 years ago

While the Registry indeed does not work as advertised I am not sure if high risk is the correct here? As per the definition "Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals)." I don't think that applies here. We also see no way Proxy owner can change and will not be registered actually can happen which would be the only scenario there is a loss of funds.

I am quite sure asset can be lost if the owner cannot do owner stuff and a non-owner can do owner stuff. Also see related PoC in e.g. #154, #67, #69

m19 commented 2 years ago

@gzeoneth Thanks, I get it now, #154 describes it much better. Yes, this is definitely a high-risk issue then.