LibDiamond.diamondCut() should check diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] != 0

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L100-L103
https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L71-L79
https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L83-L90

Vulnerability details

Impact

Normally, diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] will be set in LibDiamond.proposeDiamondCut(). Then in LibDiamond.diamondCut(), it checks that diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] < block.timestamp.

However, LibDiamond.rescindDiamondCut() will set diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] to 0. Which can easily pass the check in diamondCut(). But rescindDiamondCut should rescind _diamondCut. In conclusion, using rescindDiamondCut() can easily bypass the delay time.

Moreover, if proposeDiamondCut() has never been called, the check for delay time is always passed.

Proof of Concept

diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] will be set in LibDiamond.proposeDiamondCut()
https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L71-L79

  function proposeDiamondCut(
    IDiamondCut.FacetCut[] memory _diamondCut,
    address _init,
    bytes memory _calldata
  ) internal {
    uint256 acceptance = block.timestamp + _delay;
    diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] = acceptance;
    emit DiamondCutProposed(_diamondCut, _init, _calldata, acceptance);
  }

Then in LibDiamond.diamondCut(), it checks that diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] < block.timestamp
https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L100-L103

  function diamondCut(
    IDiamondCut.FacetCut[] memory _diamondCut,
    address _init,
    bytes memory _calldata
  ) internal {
    require(
      diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] < block.timestamp,
      "LibDiamond: delay not elapsed"
    );
    …
  }

However, LibDiamond.rescindDiamondCut() will set diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] to 0. Which can easily pass the check in diamondCut()
https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L83-L90

  function rescindDiamondCut(
    IDiamondCut.FacetCut[] memory _diamondCut,
    address _init,
    bytes memory _calldata
  ) internal {
    diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] = 0;
    emit DiamondCutRescinded(_diamondCut, _init, _calldata);
  }

diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] = 0 < block.timestamp

Tools Used

None

Recommended Mitigation Steps

Add another check in diamondCut

  function diamondCut(
    IDiamondCut.FacetCut[] memory _diamondCut,
    address _init,
    bytes memory _calldata
  ) internal {
    require(
      diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] < block.timestamp && diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] != 0,
      "LibDiamond: delay not elapsed"
    );
    …
  }

[jakekidd]

Resolved by connext/monorepo@cde1353

[0xleastwood]

I believe this issue to be valid but of medium severity as it requires a malicious or compromised governance. This issue would allow the protocol's admin to propose and execute any arbitrary data within the same transaction.