6.1 Cancellation in GaiaTimelock can't be called from the GaiaGovernor
The cancellation of the proposal is currently done in two places. The first cancellation is reachable by the public function GaiaGovernor.cancel(...) and can be called only before the voting period starts. The second proposal cancellation is reachable by the GaiaTimelock.cancel(...) and can be called when the voting period has ended, and the proposal is already in the queue. The GaiaGovernor.cancel(...) calls GovernorTimelockControl._cancel(...) which overrides Governor._cancel(...) and adds additional functionality:
function _cancel(
address[] memory targets,
uint256[] memory values,
bytes[] memory calldatas,
bytes32 descriptionHash
) internal virtual override returns (uint256) {
uint256 proposalId = super._cancel(targets, values, calldatas, descriptionHash);
// @audit: the additional logic that should allow for canceling the proposal after the voting has been done by calling the GaiaTimelock.cancel(....)
bytes32 timelockId = _timelockIds[proposalId];
if (timelockId != 0) {
// cancel
_timelock.cancel(timelockId);
// cleanup
delete _timelockIds[proposalId];
}
return proposalId;
}
This additional logic, however, is not reachable because it requires the proposal to be in the queue (after voting), but the public function from where it is called GaiaGovernor.cancel(...) requires the proposal to be before the voting.
function cancel(
address[] memory targets,
uint256[] memory values,
bytes[] memory calldatas,
bytes32 descriptionHash
) public virtual returns (uint256) {
uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash);
// @audit: the cancellation may only be called during the "Pending" state, which is before the voting period
_validateStateBitmap(proposalId, _encodeStateBitmap(ProposalState.Pending));
if (_msgSender() != proposalProposer(proposalId)) {
revert GovernorOnlyProposer(_msgSender());
}
return _cancel(targets, values, calldatas, descriptionHash);
}
To allow the cancellation of a proposal when it's already queued, the owner may grant the canceller role to another trusted address. This is, however not recommended by OpenZeppelin and stated in the comment in GaiaGovernorTimelock:
/**
* WARNING: Setting up the TimelockController to have additional proposers or cancellers besides the governor is very
* risky, as it grants them the ability to: 1) execute operations as the timelock, and thus possibly performing
* operations or accessing funds that are expected to only be accessible through a vote, and 2) block governance
* proposals that have been approved by the voters, effectively executing a Denial of Service attack.
*/
Recommendation
If the cancellation in the GaiaTimelock is not necessary, there is no need to take any action. However, if it should be reachable, consider adding another public function in GaiaGovernor that would allow calling cancellation on different states than "Pending". Alternatively, the trusted address may be granted the canceller role to call GaiaTimelock.cancel(...) directly in emergency situations. Note, however, that this solution brings DOS risk if such an address is compromised, as stated in the OZ comment above.
Resolution
Discovered a code segment that does not introduce security vulnerabilities, maintained for potential future architectural considerations.
6.2 Safeguarding Contract Operations by Disabling Ownership Renouncement
The GaiaToken contract inherits from OpenZeppelin's Ownable contract, which provides functionality for the owner to renounce ownership. Once the ownership is renounced, the contract is left without an owner, any functionality restricted to the owner would become permanently inaccessible. For example, GaiaToken extends the Pausable contract, which provides pause and unpause functionalities that can only be executed by the owner.
// @audit this functionality should be disabled
function renounceOwnership() public virtual onlyOwner {
_transferOwnership(address(0));
}
Recommendation
To protect the token from accidental renouncement of ownership, it is advisable to explicitly prevent any attempts to renounce ownership. This can be achieved by overriding the renounceOwnership function in the GaiaToken contract. The implementation below ensures the continuity and integrity of pausable operations within the contract:
function renounceOwnership() public override onlyOwner {
revert("Ownership cannot be renounced");
}
Update from Nethermind Security
Commit c4afda4.
6.3 Signers cannot cancel permit signatures before the deadline in GaiaToken
GaiaToken permit signature allows a signer to create an EIP-712 signature. However, once the signature is generated, the signer cannot cancel it before the deadline. This limitation arises because the contract is based on OpenZeppelin's ERC20Permit, which does not include an external function for manually increasing the nonce. Instead, it only provides the internal _useNonce function.
Recommendation
To enable signers to cancel their signatures, implement an external function that allows them to directly consume their nonce, effectively invalidating the signature. For example:
function consumeNonce() external {
_useNonce(msg.sender);
}
Update from Nethermind Security
Commit c4afda4.
6.4 GaiaToken cannot be paused
The GaiaToken contract incorporates an emergency stop mechanism using OpenZeppelin's Pausable contract. The Pausable module provides internal functions to activate and deactivate the paused state, which should only be triggered by an authorized account. However, the GaiaToken contract does not expose external or public functions for utilizing the pausing mechanism.
Recommendation
Add external functions that invoke the internal functions to modify the contract's state. Since GaiaToken inherits from the Openzeppelin's Ownable contract, these functions should be restricted to execution exclusively by the contract owner.
Update from Nethermind Security
Commit c4afda4.
6.5 GaiaToken extends Pausable but does not add pausable functionality
A pause feature enables the contract owner or authorized callers to temporarily suspend operations of the contract. The GaiaTokenEth contract extends the Pausable contract but does not implement the pause mechanism on any function, i.e., whenNotPaused and whenPause modifiers.
Recommendation
Add the operations that should be temporarily halted in the contract in case of an emergency.
Resolution
Fixed
Update from Nethermind Security
In the first round of reaudit, the issue was not fixed. We replied with the following consideration: “the issue is not fixed. To address the described problem, the functions that transfer, mint, or burn tokens should have a whenNotPaused modifier. Alternatively, this modifier can be added to the _udpate(...) function, which is a common function for all those“. In the second round of reaudit, the issues was fixed in the commit c4afda4.
6.6 Important deployment information
The deployment of the GaiaGovernor and GaiaTimelock is difficult since both contracts require each others' addresses. The recommended way of deployment is to first deploy GaiaTimelock and put empty arrays for executors and proposers in the constructor. Then after the deployment of the GaiaGovernor, admin should call the GaiaTimelock to grant the executor and proposer roles to the GaiaGovernor contract.
Recommendation
Consider following the proposed scenario during deployment. Additionally read the discussion of this topic here.
Resolution
The deployment of GaiaTimelock does not require the address of the GaiaGovernor contract.
Update from Nethermind Security
During the deployment of GaiaTimelock the proposers and executors are needed to grant proposer, canceller and executor roles (link to the OpenZeppelin implementation). The GaiaGovernor should hold those roles according to the current implementation, therefore it's address is needed during the GaiaTimelock construction.
6.7 Quorum value may be incorrect because of bridging
To accept the proposal, a quorum of votes must be reached. The quorum is set by the protocol owner and is calculated as the sum of "for" and "abstain" votes proportional to all the voting power (total minted GAIA tokens). However, with the current setup, the 40% of all minted tokens are predicted to be bridged to the Base and used there for restaking. The bridged GAIA tokens would be frozen in the bridge contract but would still be accounted for in the calculation of the quorum. That leads to an incorrect quorum. Consider the following example: the owner mints 1000 GAIA tokens. The quorum is set to 10%, which is 100 tokens. 40% of those tokens are bridged to Base, leaving 600 tokens on Ethereum. To reach quorum during voting, the proposal must get at least 100 "for/abstain" votes, which constitutes 100/600, which makes a real quorum at 16.6% instead of the initial 10%. Note that the number of bridged tokens may change over time, changing the value of the real quorum.
Recommendation
This is a business decision from the client.
Resolution
No modifications for now. The quorum threshold can be manually lowered.
6.8 GaiaTimelock could allow anyone to execute proposals
The GaiaTimelock contract extends OpenZeppelin's TimelockController contract, which enforces a delay between queueing and executing an operation. This contract introduces the flexibility to operate in two modes: only role and open role. The onlyRoleOrOpenRole modifier is used in the execute and executeBatch functions, as shown below:
function execute(
address target,
uint256 value,
bytes calldata payload,
bytes32 predecessor,
bytes32 salt
) public payable virtual onlyRoleOrOpenRole(EXECUTOR_ROLE) { ... }
function executeBatch(
address[] calldata targets,
uint256[] calldata values,
bytes[] calldata payloads,
bytes32 predecessor,
bytes32 salt
) public payable virtual onlyRoleOrOpenRole(EXECUTOR_ROLE) { ...}
Granting EXECUTOR_ROLE to the zero address ( address(0)) enables anyone to execute proposals. The GaiaTimelock contract should prohibit setting open roles unless explicitly allowed during deployment. Currently, as presented below, executors are set without validation, which could inadvertently permit open roles:
constructor(
uint256 minDelay,
address[] memory proposers,
address[] memory executors,
address admin
) TimelockController(minDelay, proposers, executors, admin) {}
Recommendation
You can follow the guidelines explained here. Make sure that this role is set before the deployer renounces its admin privileges in favor of GaiaTimelock contract itself (for details see TimelockController roles.
Resolution
We recognize that role validation is critical and guides future developers to implement proper access controls.
