diamondCut doesn't validate _init and _calldata

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/LibDiamond.sol#L95-L118

Vulnerability details

Malicious or compromised governance can send a different _init and _calldata payload than the one that was proposed. This is dangerous as they will be used for a delegatecall operation. An attacker can pretend to propose a safe upgrade and later execute a delegatecall to steal funds or selfdestruct the system.

Proof of Concept

• Malicious governor proposed a normal update with empty payload for _init and _calldata.
• After the update delay has passed, the governor executes the update with _init pointing to a malicious contract, stealing funds from the system.

Recommended Mitigation Steps

Consider hashing _init and _calldata alongside _diamondCut as the acceptanceTimes key, to ensure the execution will use the same payload.

// @audit Apply to rescindDiamondCut() and diamondCut() as well.
function proposeDiamondCut(
    IDiamondCut.FacetCut[] memory _diamondCut,
    address _init,
    bytes memory _calldata
) internal {
    uint256 acceptance = block.timestamp + _delay;
    diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut, _init, _calldata))] = acceptance;
    emit DiamondCutProposed(_diamondCut, _init, _calldata, acceptance);
} 

[jakekidd]

Malicious or compromised governance can send a different _init and _calldata payload than the one that was proposed. This is dangerous as they will be used for a delegatecall operation.

Right now, "governance" is just an Owner role. However, I am still concerned with whether there is, hypothetically, a security issue here when governance is deployed for the protocol in the future.

Additionally worth noting that the implementation here differs from the boilerplate EIP-2535 by adding the propose route, so this isn't a redundant issue with the core proposal.

Because of this, marking as valid. The mitigation step works fine for this.

[jakekidd]

Duplicate of #241