code-423n4 / 2022-10-zksync-findings

3 stars 0 forks source link

Incorrect parameter of `EmergencyDiamondCutApproved` event #313

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/DiamondCut.sol#L113 https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/DiamondCut.sol#L119

Vulnerability details

Description

There is a function approveEmergencyDiamondCutAsSecurityCouncilMember in DiamondCutFacet contract. It contains the following logic:

/// @notice Gives another approval for the instant upgrade (diamond cut) by the security council member
/// @param _diamondCutHash The hash of the diamond cut that security council members want to approve. Needed to prevent unintentional approvals, including reorg attacks
function approveEmergencyDiamondCutAsSecurityCouncilMember(bytes32 _diamondCutHash) external {
    require(s.diamondCutStorage.securityCouncilMembers[msg.sender], "a9"); // not a security council member
    uint256 currentProposalId = s.diamondCutStorage.currentProposalId;
    require(s.diamondCutStorage.securityCouncilMemberLastApprovedProposalId[msg.sender] < currentProposalId, "ao"); // already approved this proposal
    s.diamondCutStorage.securityCouncilMemberLastApprovedProposalId[msg.sender] = currentProposalId;

    require(s.diamondCutStorage.proposedDiamondCutTimestamp != 0, "f0"); // there is no proposed diamond cut
    require(s.diamondCutStorage.proposedDiamondCutHash == _diamondCutHash, "f1"); // proposed diamond cut do not match to the approved
    uint256 securityCouncilEmergencyApprovals = s.diamondCutStorage.securityCouncilEmergencyApprovals;
    s.diamondCutStorage.securityCouncilEmergencyApprovals = securityCouncilEmergencyApprovals + 1;

    emit EmergencyDiamondCutApproved(
        msg.sender,
        currentProposalId,
        securityCouncilEmergencyApprovals,
        _diamondCutHash
    );
}

When the call of such a function does not revert the number of security council emergency approvals will be increased by 1. But the event EmergencyDiamondCutApproved that is emitted has a parameter securityCouncilEmergencyApprovals which will be equal to the number of security council emergency approvals before the function call, not after it.

Recommended Mitigation Steps

Use the following logic when updating the number of approvals and emitting the event:

uint256 newSecurityCouncilEmergencyApprovals = s.diamondCutStorage.securityCouncilEmergencyApprovals + 1;
s.diamondCutStorage.securityCouncilEmergencyApprovals = newSecurityCouncilEmergencyApprovals;

emit EmergencyDiamondCutApproved(
    msg.sender,
    currentProposalId,
    newSecurityCouncilEmergencyApprovals,
    _diamondCutHash
);
c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #310

GalloDaSballo commented 1 year ago

R

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

GalloDaSballo commented 1 year ago

Closing as not enough points

c4-judge commented 1 year ago

GalloDaSballo marked the issue as grade-c