Governance

Proof of concept: PoC

When unstaking GMX and _shouldReduceBnGmx = true, a proportionate amount of the user's bnGMX wallet balance is burnt.

A user can avoid the loss of their bnGMX (any amount beyond what is claimed in _unstakeGmx) by transferring the token to another account they control.

After unstaking, the sent bnGMX could be returned. Consequently, a user will have more multiplier points than intended, which will lead to more voting power.

Consider restricting the transfer of bnGMX for non-handler accounts. Furthermore, clearly document the intended behavior regarding bnGMX slashing upon unstaking GMX.

GMX Team: bnGMX.inPrivateTransferMode is not currently set to true, agree that it should be set to true.

The _transferOutETHWithGasLimitFallbackToWeth and _transferOutETH functions in GMX V1 are used to transfer ether.

Neither of these functions utilizes assembly to avoid copying return data into memory.

This allows any contract interacting with GMX V1 to maliciously return a large amount of data, which will end up costing more gas than anticipated and could force the keepers to run a deficit.

Use a low-level call to avoid loading the return data into memory: assembly { success := call(gasLimit, receiver, amount, 0, 0, 0, 0) }.

GMX Team: The resolution was implemented.

In the GovToken and ProtocolGovernance contracts, the CLOCK_MODE function validates that the clock function returns the block.timestamp, however it should be validated against the Chain.currentTimestamp to avoid failures due to L2 timestamp drift.

Validate the result of the clock function against the Chain.currentTimestamp().

GMX Team: The resolution was implemented.

Proof of concept: PoC

When syncing an account's voting power, the user's staked amounts are compared against their current governance token wallet holdings. If the governance holdings are less, the appropriate amount is minted.

This can be exploited if a user transfers their governance tokens to another account, triggers a sync, and then those governance tokens are minted again. This way, a user can generate infinite votes and move forward malicious proposals.

Ensure that the governance token used cannot be freely transferred.

GMX Team: GovToken.sol would be used, only contracts with the role GOV_TOKEN_CONTROLLER would be able to make transfers.

In the acceptTransfer function, the _compound invocation will always trigger a syncing of the voting power for the _sender. However, the voting is once again synced after stake balances are transferred at the end of the acceptTransfer function.

Consider adding a boolean parameter to the _compound function to specify whether the voting power should be synced. In the case of an account transfer this boolean would be false to avoid unnecessary syncs to save gas. In all other cases the boolean value would be true.

GMX Team: Acknowledged.

In the _stakeBnGmx function the _syncVotingPower invocation will be missed if the currentBnGmxAmount is greater than the maxAllowedBnGmxAmount.

There is no net effect as the _syncVotingPower function will always be called for the _account later for every instance where the _stakeBnGmx function is used.

However, this poses a risk if the _stakeBnGmx function were to be used without syncing the voting power of the _account afterwards.

Consider removing the _syncVotingPower invocation from the _stakeBnGmx function as it is redundant for all cases where the _stakeBnGmx function is currently used.

Otherwise be sure to sync the voting power in the case where the currentBnGmxAmount is greater than the maxAllowedBnGmxAmount and the _stakeBnGmx function early returns.

GMX Team: The recommendation was implemented.

In the _stakeGmx function it is unnecessary to sync the voting power for the funding account as the funding account will provide unstaked tokens, while the voting power relies on staked tokens.

Therefore the voting power of the funding account cannot be affected by the _stakeGmx function.

Additionally, in the only case where the _stakeGmx function is used with a _fundingAccount that is different from the account is in the acceptTransfer function where the voting power is redundantly synced for the _sender at the end.

Remove the syncing logic for the _fundingAccount in the _stakeGmx function as it is unnecessary.

GMX Team: The recommendation was implemented.

Calling the function signalTransfer requires that msg.sender has given the _receiver allowance equivalent to or more than the balance of msg.sender when inStrictTransferMode is set to true.

The original msg.sender can decrease the approval prior to the receiver accepting the transfer, causing the function acceptTransfer to revert in the case where the transfer is not being performed by a handler.

When the transfer is performed by the handler, transferFrom does not check the allowance on the BaseToken.

Consider removing the inStrictTransferMode check since the transfers are being done by handlers, or clearly document its intended behavior.

GMX Team: The reason for inStrictTransferMode is mainly to prevent phishing, we have observed phishing scams that utilize signalTransfer, because no approval is needed, there is less warning / less alerting of the user from their wallet

In the deploy file for the ProtocolGovernor contract, 30_000 is used as a proposalThreshold. However, this is a minuscule amount of governance tokens, as the governance token is configured with 18 decimals.

Be sure to configure a reasonable minimum threshold with 18 decimals for proposals in production as the proposalThreshold is in a governance token amount, which has 18 decimals of precision.

GMX Team: The recommendation was implemented.