Lack of check for contract existence in `DiamondProxy`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/DiamondProxy.sol#L19-L54

Vulnerability details

Impact

Not checking if the delegated contract exists can result silent failures for the caller, causing unexpected contract funtionality and break third-party integrations.

Proof of Concept

• The proxy delegates to a address which is not a contract
• DiamondProxy fallback uses delegatecall and does not check for contract existance
• The fallback will return success, causing a slient failure

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/DiamondProxy.sol#L19-L54

Trail of bits has an article about this issue (search for "No contract existence check").

Note that OpenZeppelin checks for contract existance for Initializable inside proxy (not diamond).

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/utils/Initializable.sol#L86

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L41

Recommended Mitigation Steps

Since the project is adding an in-house implementation of the Diamond Standard, consider adding a check for contract existance. E.g.

diff --git a/DiamondProxy.sol.orig b/DiamondProxy.sol
index 6240c9a..706c23a 100644
--- a/DiamondProxy.sol.orig
+++ b/DiamondProxy.sol
@@ -29,6 +29,8 @@ contract DiamondProxy {
         require(facetAddress != address(0), "F"); // Proxy has no facet for this selector
         require(!diamondStorage.isFrozen || !facet.isFreezable, "q1"); // Facet is frozen

+        require(facetAddress.code.length > 0, "");
+
         assembly {
             // The pointer to the free memory slot
             let ptr := mload(0x40)

[c4-judge]

GalloDaSballo marked the issue as primary issue

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[miladpiri]

This issue does not have bad impact and can not adversly impact on the user or protocol. Moreover, the probablity is very low, because the facet and it's selectors are added in the diamondCut, so there are a lot of middle checks that prevents such issue. QA is a fair severity!

[c4-sponsor]

miladpiri marked the issue as sponsor acknowledged

[GalloDaSballo]

Agree that the submission is missing a lossy impact, will downgrade to QA - Low

L

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-10-zksync-findings/issues/361