Assessment reports>H20 vlPSDN>Informational findings>Centralization risk: reward token recovery
Category: Coding Mistakes

Centralization risk: reward token recovery

Informational Severity
Informational Impact
Low Likelihood

Description

  • In the recoverERC20 function, the owner can recover any ERC20 token excluding lockToken.

  • In the recoverERC721 function, the owner can recover any ERC721 token.

Impact

In the event that the owner's private key is compromised, an attacker could potentially steal all reward tokens that have not yet been claimed by users by whitelisting a token and calling the recoverERC20 function.

The changeRecoverWhitelist function does contain a check to prevent the owner from removing the governance token:

/**
 *  @notice  Add or remove a token from recover whitelist,
 * cannot whitelist governance token
 *  @dev Only contract owner are allowed. Emits an event
 * allowing users to perceive the changes in contract rules.
 * The contract allows to whitelist the underlying tokens
 * (both lock token and rewards tokens). This can be exploited
 * by the owner to remove all funds deposited from all users.
 * This is done bacause the owner is mean to be a multisig or
 * treasury wallet from a DAO
 *  @param flag: set true to allow recover
 */
function changeRecoverWhitelist(address tokenAddress, bool flag) external onlyOwner {
    if (tokenAddress == lockToken) revert CannotWhitelistLockedToken(lockToken);
    if (tokenAddress == rewardTokens[0]) revert CannotWhitelistGovernanceToken(rewardTokens[0]);
    whitelistRecoverERC20[tokenAddress] = flag;
    emit ChangeERC20Whiltelist(tokenAddress, flag);
}

However, the check is ineffective because the owner can simply remove all tokens from rewardTokens using the removeReward function. This allows the owner to steal all reward funds.

Recommendations

  • Use a multi-signature address wallet; this would prevent an attacker from causing economic damage if a private key were compromised.

  • Set critical functions behind a timelock to catch malicious executions in the case of compromise.

  • Prohibit withdrawal of reward tokens.

Remediation

H20 added a new role called PAUSE_SETTER_ROLE that is responsible for administering the pause and unpause functionality. Additionally, they have implemented the use of TimeLockController for ownership in commit 77d735f0.

Zellic © 2023Back to top ↑