code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-10-zksync/blob/456078b53a6d09636b84522ac8f3e8049e4e3af5/ethereum/contracts/zksync/facets/DiamondCut.sol#L22

Vulnerability details

Impact

Not knowing the code in the initAddress and delegatecalling to it can have huge risk on the protocol.

Proof of Concept

A malicious governor proposes a valid proposal with nonzero initAddress and initCalldata.

function proposeDiamondCut(Diamond.FacetCut[] calldata _facetCuts, address _initAddress) external onlyGovernor {
        require(s.diamondCutStorage.proposedDiamondCutTimestamp == 0, "a3"); // proposal already exists

        // NOTE: governor commits only to the `facetCuts` and `initAddress`, but not to the calldata on `initAddress` call.
        // That means the governor can call `initAddress` with ANY calldata while executing the upgrade.
        s.diamondCutStorage.proposedDiamondCutHash = keccak256(abi.encode(_facetCuts, _initAddress));
        s.diamondCutStorage.proposedDiamondCutTimestamp = block.timestamp;
        s.diamondCutStorage.currentProposalId += 1;

        emit DiamondCutProposal(_facetCuts, _initAddress);
    }

But the initAddress is an address that does not have any code before execution of the proposal. So, just before executing the proposal, the malicious governor deploys a contract on the address of initAddress by using create2. At the end of proposal execution, the diamond proxy delegates call to this initAddress, and can exploit the protocol.

function _initializeDiamondCut(address _init, bytes memory _calldata) private {
        if (_init == address(0)) {
            require(_calldata.length == 0, "H"); // Non-empty calldata for zero address
        } else {
            // Do not check whether `_init` is a contract since later we check that it returns data.
            (bool success, bytes memory data) = _init.delegatecall(_calldata);
            require(success, "I"); // delegatecall failed

            // Check that called contract returns magic value to make sure that contract logic
            // supposed to be used as diamond cut initializer.
            require(data.length == 32, "lp");
            require(abi.decode(data, (bytes32)) == DIAMOND_INIT_SUCCESS_RETURN_VALUE, "lp1");
        }
    }

Tools Used

Recommended Mitigation Steps

It should be checked that initAddress has code during creating proposal.

GalloDaSballo commented 1 year ago

Contingent on externalities Assumes you can have different logic at same address No coded POC

Looks off tbh

miladpiri commented 1 year ago

As it helps to the transparency of the protocol, it worth QA!

c4-sponsor commented 1 year ago

miladpiri marked the issue as disagree with severity

c4-sponsor commented 1 year ago

miladpiri marked the issue as sponsor confirmed

c4-sponsor commented 1 year ago

miladpiri marked the issue as sponsor acknowledged

miladpiri commented 1 year ago

It should be noted that a malicious governor can deploy a proxy contract on initAddress or deploy a contract with selfdestruct functionality on initAddress, so in this case even with nonzero code size check on initAddress, the malicious governor can exploit the protocol. But, at least this check helps to the transparency of the protocol, because users notice that a contract with selfdestruct or proxy is deployed on ìnitAddress, so the users will conclude that something is maybe wrong. So, the users can take a risk and hold their fund in the protocol or withdraw.

GalloDaSballo commented 1 year ago

Per the comment from the sponsor, am downgrading to QA - Low

GalloDaSballo commented 1 year ago

L