The diamond shall be monitored externally to remove the need of trust to developers. If a timelock can be bypassed, it poses a threat as people who weren't trusted can exploit the system.
Additionally, the contract can immediately perform any delegatecall of owner's choice.
Proof of Concept
require(
diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] < block.timestamp,
"LibDiamond: delay not elapsed"
);
The above code isn't enough verification if the delay elapsed, if the hash was never used before, then this check will pass as the value read from storage is 0. This means that th change hasn't been proposed, but that isn't checked.
Tools Used
Manual analysis
Recommended Mitigation Steps
Require that the value read is nonzero and less than the current timestamp.
LayneHaber
commented
2 years ago
Lines of code
https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/DiamondCutFacet.sol#L16-L29 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L94-L118 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L222-L240
Vulnerability details
Impact
The diamond shall be monitored externally to remove the need of trust to developers. If a timelock can be bypassed, it poses a threat as people who weren't trusted can exploit the system.
Additionally, the contract can immediately perform any delegatecall of owner's choice.
Proof of Concept
The above code isn't enough verification if the delay elapsed, if the hash was never used before, then this check will pass as the value read from storage is 0. This means that th change hasn't been proposed, but that isn't checked.
Tools Used
Manual analysis
Recommended Mitigation Steps
Require that the value read is nonzero and less than the current timestamp.