GovernanceFacet Manual Review Findings

GFT-01M: Inexistent Direct Invocation Protection

Type	Severity	Location
Logical Fault		GovernanceFacet.sol:L19-L26

Description:

The referenced function is meant to act as a form of state initiation yet does not impose any access control on the logic contract's implementation.

Impact:

While inconsequential in this instance, it is always best practice to not allow the logic contract implementation to be tampered with.

Example:

Copy
14/**
15 * @notice sets the state for the Governance facet
16 * @param data_ abi encoded data - the list of governance members.
17 * @dev This state method is never attached on the diamond
18 */
19function state(bytes memory data_) external {
20    (address[] memory members) = abi.decode(data_, (address[]));
21    require(members.length > 0, "Governance: member list empty");
22    for (uint256 i = 0; i < members.length; i++) {
23        LibGovernance.updateMember(members[i], true);
24        emit MemberUpdated(members[i], true);
25    }
26}

Recommendation:

We advise the function to be solely accessible via delegatecall instructions by storing the address(this) evaluation to an immutable contract variable and ensuring that address(this) != self when the GovernanceFacet::state function is invoked, guaranteeing that the function can only be accessed via delegatecall instructions.

Alleviation (54fd570de24631ca65a7cea022aebe43225a08c7):

Direct call protection was introduced per our recommendation, utilizing a contract-level immutable variable assigned to address(this) during the contract's deployment.

GFT-02M: Inexistent Conformity of Checks-Effects-Interactions Pattern

Type	Severity	Location
Language Specific		GovernanceFacet.sol:L45, L46, L49

Description:

The Checks-Effects-Interactions (CEI) pattern is not adhered to by the GovernanceFacet::updateMember implementation as it will attempt to disburse rewards to the account_ prior to removing them as a member of the governance system.

Impact:

A malicious user could in theory re-enter the contract and cause an initially invalid member removal to succeed while acquiring a disproportionate amount of rewards.

The likelihood of this vulnerability is low given that the relevant governance signatures would need to have been procured, however, its impact is high as funds would be lost thereby culminating to a medium severity grade.

Example:

Copy
28/**
29 *  @notice Adds/removes a member account
30 *  @param account_ The account to be modified
31 *  @param status_ Whether the account will be set as member or not
32 *  @param signatures_ The signatures of the validators authorizing this member update
33 *  @dev Upon removal we also send out the validator's accumulated reward
34 */
35function updateMember(address account_, bool status_, bytes[] calldata signatures_)
36    onlyConsensusNonce(computeMemberUpdateMessage(account_, status_), signatures_)
37    external override
38{
39    require(account_ != address(0));
40
41    if (status_) {
42        LibFeeCalculator.addNewMember(account_);
43    } else {
44        uint256 claimableAmount = LibFeeCalculator.claimReward(account_);
45        (bool success, bytes memory returndata) = account_.call{value: claimableAmount}("");
46        require(success, string(returndata));
47    }
48
49    LibGovernance.updateMember(account_, status_);
50    emit MemberUpdated(account_, status_);
51}

Recommendation:

We advise this to be prohibited as a re-entry prior to the revocation of the governance status can be exploited, for example by:

• Invoking GovernanceFacet::updateMember with a non-governance member, causing LibFeeCalculator::claimReward to yield an abnormally high number
• Re-enter the contract to update the member as an actual governance member via the GovernanceFacet::updateMember function

In the above scenario, the original removal of the inexistent governance member would succeed while they would acquire a disproportionate value of rewards by the contract that would hurt the overall Diamond system.

Alleviation (54fd570de24631ca65a7cea022aebe43225a08c7):

The code was updated to issue the native payment at the end of the function after the member's status has been updated, alleviating this exhibit as advised.

GFT-03M: Improper Mandation of Fund Distribution

Type	Severity	Location
Logical Fault		GovernanceFacet.sol:L45, L46

Description:

The GovernanceFacet::updateMember function will disburse any pending rewards to the account_ being removed, thereby permitting the account to sabotage the removal operation and prevent itself from ever being removed.

Impact:

We consider this exhibit to be of major severity as a significant invariant of the system is breached; a user can sabotage removal operations of themselves and thus become a permanent member of the contract's governance structure.

Example:

Copy
28/**
29 *  @notice Adds/removes a member account
30 *  @param account_ The account to be modified
31 *  @param status_ Whether the account will be set as member or not
32 *  @param signatures_ The signatures of the validators authorizing this member update
33 *  @dev Upon removal we also send out the validator's accumulated reward
34 */
35function updateMember(address account_, bool status_, bytes[] calldata signatures_)
36    onlyConsensusNonce(computeMemberUpdateMessage(account_, status_), signatures_)
37    external override
38{
39    require(account_ != address(0));
40
41    if (status_) {
42        LibFeeCalculator.addNewMember(account_);
43    } else {
44        uint256 claimableAmount = LibFeeCalculator.claimReward(account_);
45        (bool success, bytes memory returndata) = account_.call{value: claimableAmount}("");
46        require(success, string(returndata));
47    }
48
49    LibGovernance.updateMember(account_, status_);
50    emit MemberUpdated(account_, status_);
51}

Recommendation:

We advise the code to instead perform a safe native call, allowing it to fail (i.e. not validating the success flag) as well as forwarding a fixed gas stipend that is low enough to prevent gas bombing attacks.

Alleviation (54fd570de2):

The code was updated to perform a call with a fixed gas stipend of 2_300, however, this is insufficient in preventing a malicious recipient from reverting the transaction.

We advise usage of the ExcessivelySafeCall library which will ensure that the call will be safely performed and will not revert.

Alleviation (d7618eed4e):

The code was updated to utilize the ExcessivelySafeCall library per our recommendation, ensuring that the account_ cannot force the transaction to revert by a malicious return payload.

As a result, we consider this exhibit fully alleviated as the refund mechanism can no longer result in a revert error.