Protocol - Plural Energy

The Owner can recover funds from a compromised account by burning and re-minting tokens. However, it was identified that the recover() function was implemented using the mint() function, which reverts if the total supply is not 0. Therefore, tokens could only be recovered if only one wallet held the entire total supply.

Note that it was also identified that the recover() function does not verify if the wallet is sanctioned.

Code Location

The recover() function uses the AssetToken's mint() function instead of OpenZeppelin's ERC20._mint():

src/AssetToken.sol

function recover(address from, uint256 amount) public override onlyOwner {
    /// @dev: to start, recovery can consist of blocking transfer for a wallet, either through sanctioning
    ///       or role removal, then once that's done we can burn and remint
    /// @dev: require multisig here: e.g. 2/3 multisig with keys held by admin, regulators, and a trusted third party
    address owner = owner();
    _burn(from, amount);
    mint(owner, amount);
    emit Recovered(from, owner, amount);
}

The mint() function reverts if the total supply is not 0:

src/AssetToken.sol

function mint(address to, uint256 amount) public override onlyOwner {
    compliance.authorizeTransfer(msg.sender, to, address(this), amount);
    require(totalSupply() == 0, "Not allowing additional minting");
    require(decimals() <= 18, "Max decimals for token is 18");
    require(amount <= 1e9 * 10 ** decimals(), "Limit tokens to 1 billion to prevent overflow");
    super._mint(to, amount);
    IRewardsManager(rewardsManager).signalAssetTokensMinted();
}

The recover() function reverts:

SOLVED: The \client team solved the issue in commits 860295a and bbc76ac by modifying the recover() function to allow the Owner to burn tokens from users.

A wallet can be linked to a parent wallet by the Owner with the linkWallet() function of the Compliance contract. The contract assumes that it is not possible for a wallet to be both a child and a parent. Otherwise, there would be cases where the sanction checks could be bypassed. However, because of the improper validation, it is possible to link more than two wallets.

Code Location

Wallets can be linked using the linkWallet() function.

src/Compliance.sol

    function linkWallet(
        address child,
        address parent
    ) external override onlyOwner {
        if (hasRole(Roles.ROLE_SANCTIONED, parent)) {
            revert ParentSanctioned();
        }
        if (linkedWallets[parent] != address(0)) {
            revert ParentWalletIsChild();
        }
        linkedWallets[child] = parent;
        emit LinkedWallet(parent, child);
    }

Reversing the linking order of the wallets bypasses the validation check:

SOLVED: The \client team solved the issue in commit a44aa74 by adding the validation.

It was identified that the updateTokenImplementation() function in the AssetFactory contract calls the upgradeTo() function of the AssetToken contract, but the upgradeTo() function can only be called by the Owner of the AssetToken contract. Because the Owner is a wallet that manages the AssetToken contract and the AssetFactory does not have the required permission, the updateTokenImplementation() function reverts.

Code Location

src/AssetFactory.sol

    function updateTokenImplementation(
        address token,
        address newImplementation
    ) external override onlyOwner {
        if (newImplementation == address(0)) {
            revert ZeroImplementationAddress();
        }

        AssetToken(token).upgradeTo(newImplementation);
        emit UpdatedTokenImplementation(newImplementation);
    }

src/AssetToken.sol

    function upgradeTo(address newImplementation) public override onlyOwner {
        super.upgradeTo(newImplementation);
    }

SOLVED: The \client team solved the issue in commit a44aa74 by configuring the AssetFactory as Owner during initialization and using a different role to manage the AssetToken contract.

It was identified that the depositRewards() function in the RewardsManagerConstant contract uses an improper distribution period overlapping validation. Therefore, it is possible to start a new reward period before the end of the current one, despite validation check and documentation. However, it is important to note that the rewards did not stack, as the previous rate and duration is overwritten with the new one, and the new reward is not applied retroactively.

Code Location

The depositRewards() function compares the current block timestamp to the time elapsed since the genesis point, and therefore this validation will always be true:

src/RewardsManagerConstant.sol

    function depositRewards(
        uint64 distributionDuration,
        uint64 rewardPerSecond
    ) external override {
        if (
            !hasRole(rewardsManagerRole, msg.sender) &&
            !hasRole(OWNER_ROLE, msg.sender)
        ) {
            revert NotRewardsManager();
        }
        if (rewardPerSecond > REWARD_PER_SECOND_MAX) {
            revert RewardRateTooHigh();
        }
        /// Prevent underflow with online avgerage calculation, we shouldn't have rewards this small
        /// anyways. At 1e6 precision, this is $0.000001/second => ~$31/year
        if (rewardPerSecond < REWARD_PER_SECOND_MIN) {
            revert RewardRateTooLow();
        }

        uint40 currentTime = uint40(block.timestamp);

        // Check that the now > accrualEnd so we don't stack dividends
        RewardsConfig memory config = _updateReward();
        if (currentTime < config.accrualEnd) {
            revert NoOverlappingRewards();
        }

        if (config.genesisTimestamp <= 0) {
            config.genesisTimestamp = currentTime;
        }

        uint64 distributionEnd = _currentOffset(config.genesisTimestamp) +
            distributionDuration;

        config.receivedRewards += rewardPerSecond * distributionDuration;
        config.accrualEnd = distributionEnd;
        config.currentPeriodRewardPerSecond = rewardPerSecond;

After 1 year, Bob should have earned 3153601 USDT. However, it was possible to start a new reward period that overwrote the original one.

SOLVED: The \client team solved the issue in commit 09c9c2f by not allowing depositing rewards until the current period ends.

Note that, in this version, it is not possible to cancel or modify the active reward period. However, the administrators can withdraw the deposited reward tokens from the contract.

It was identified that the same allowed role can be added to the asset token multiple times. This might result in unexpected behavior, for example, in a deadlock situation where removing the roles would not be possible because of an Index out of range error.

Code Location

Roles can be associated with tokens using the addAllowedRolesToTokens() and resumeRoles() functions:

src/Compliance.sol

function addAllowedRolesToTokens(
    address[] calldata assetTokens,
    string[] calldata roles
) public override onlyOwner {
    for (uint i = 0; i < assetTokens.length; i++) {
        for (uint j = 0; j < roles.length; j++) {
            tokenAllowedRoles[assetTokens[i]].push(
                keccak256(abi.encodePacked(roles[j]))
            );
        }
    }
    emit AddedRolesToTokens(assetTokens, roles);
}

src/Compliance.sol

function resumeRoles(
    address tokenAddress,
    bytes32[] calldata roles
) external override onlyOwner {
    for (uint256 i = 0; i < roles.length; i++) {
        tokenAllowedRoles[tokenAddress].push(roles[i]);
    }
    emit ResumedRoles(tokenAddress, roles);
}

src/Compliance.sol

    function _halt(address tokenAddress, bytes32 roleToRemove) internal {
        bytes32[] memory allowedRoles = tokenAllowedRoles[tokenAddress];
        uint rolesLength = allowedRoles.length;

        for (uint256 x = 0; x < rolesLength; x++) {
            if (allowedRoles[x] == roleToRemove) {
                tokenAllowedRoles[tokenAddress][x] = tokenAllowedRoles[
                    tokenAddress
                ][rolesLength - 1];
                tokenAllowedRoles[tokenAddress].pop();
            }
        }
    }

SOLVED: The \client team solved the issue in commit 3cae28c.

The Compliance contract has different roles to manage the compliance status of the wallets. However, it was identified that the addRoleToAccount(), addRoleToAccounts() and removeRoleFromAccounts(), sanctionWallets() and sanctionWallet() functions can only be used by the Owner. This limits the ROLE_MANAGER's ability to manage the permissions effectively.

Code Location

The roles are configured in the initialize() function.

src/Compliance.sol

    function initialize(address owner) public override initializer {
        if (owner == address(0)) {
            revert ZeroOwnerAddress();
        }
        __BaseAccessControlPausable__init(owner);

        _setRoleAdmin(Roles.ROLE_MANAGER, OWNER_ROLE);
        _setRoleAdmin(Roles.ROLE_KYC_APPROVED, OWNER_ROLE);
        _setRoleAdmin(Roles.ROLE_US_KYC_APPROVED, OWNER_ROLE);
        _setRoleAdmin(Roles.ROLE_KYC_ACCREDITED, OWNER_ROLE);
        _setRoleAdmin(Roles.ROLE_US_KYC_ACCREDITED, OWNER_ROLE);
        _setRoleAdmin(Roles.ROLE_KYB_APPROVED, OWNER_ROLE);
        _setRoleAdmin(Roles.ROLE_US_KYB_APPROVED, OWNER_ROLE);
        _setRoleAdmin(Roles.ROLE_PRIVATE_PLACEMENT, OWNER_ROLE);
        _setRoleAdmin(Roles.ROLE_PROTOCOL, OWNER_ROLE);
        _setRoleAdmin(Roles.ROLE_PENDING_SANCTIONED, OWNER_ROLE);
        _setRoleAdmin(Roles.ROLE_SANCTIONED, OWNER_ROLE);
    }

However, administrative functions can only be called by the Owner. For example:

src/Compliance.sol

function addRoleToAccounts(
    string calldata roleName,
    address[] calldata accounts
) external override onlyOwner {
    for (uint256 i = 0; i < accounts.length; i++) {
        addRoleToAccount(roleName, accounts[i]);
    }
}

SOLVED: The \client team solved the issue in commits df5fa09, f68bb87 and 5232415.

The ownership of the contracts can be lost as the AssetToken contract is inherited from the OwnableUpgradeable contract and its ownership can be transferred in a single-step process. The address the ownership is changed to should be verified to be active or willing to act as the owner.

Code Location

@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol

function transferOwnership(address newOwner) public virtual onlyOwner {
    require(newOwner != address(0), "Ownable: new owner is the zero address");
    _transferOwnership(newOwner);
}

@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol

function _transferOwnership(address newOwner) internal virtual {
    address oldOwner = _owner;
    _owner = newOwner;
    emit OwnershipTransferred(oldOwner, newOwner);
}

SOLVED: The \client team solved the issue in commit 971f710 by using the Ownable2StepUpgradeable library over the OwnableUpgradeable library in the AssetToken contract.

The AssetToken contract is inherited from the OwnableUpgradeable contract. The Owner of the contract is usually the account that deploys the contract. As a result, the Owner can perform some privileged functions. In the Ownable contracts, the renounceOwnership() function is used to renounce the Owner permission. Renouncing ownership before transferring would result in the contract having no Owner, eliminating the ability to call privileged functions.

Code Location

@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol

function renounceOwnership() public virtual onlyOwner {
    _transferOwnership(address(0));
}

SOLVED: The \client team solved the issue in commit f100681 by overriding the renounceOwnership() function.

It was identified that the whennotpaused modifier is not used in the Compliance contract, even though it is inherited from the BaseAccessControlPausableUpgradeable contract.

SOLVED: The \client team solved the issue in commit 514e35d by using the emergency stop pattern with the authorizeTransfer() function.

It was identified that the setRewardsManager() function in the AssetToken contract does not emit any events. As a result, it might be more difficult for blockchain monitoring systems to detect suspicious behavior related to these features.

SOLVED: The \client team solved the issue in commits 642dcce and 9c42d1e by adding events for all important operations.

The contracts use the Initializable module from OpenZeppelin, and the implementations of these contracts are not marked as initialized in the constructor. An uninitialized instance can be initialized by anyone to take over the contract. Even if it does not affect the instances deployed by the AssetFactory, it is still a good practice to prevent the initialization of the implementation contracts to avoid misuse. In the latest versions, this is done by calling the _disableInitializers function in the constructor.

SOLVED: The \client team solved the issue in commit 565ddec by including a constructor to automatically mark the contracts as initialized when they are deployed.

It was identified that the for loops employed in the Compliance , RewardsManagerConstant and RewardsManagerRetroactive contracts can be gas optimized by the following principles:

• Unnecessary reading of the array length on each iteration wastes gas.
• A postfix (e.g. i++) operator was used to increment the i variables. It is known that, in loops, using prefix operators (e.g. ++i) costs less gas per iteration than postfix operators.
• It is also possible to further optimize loops by using unchecked loop index incrementing and decrementing.

Code Location

Example function using unoptimized for loops:

src/Compliance.sol

function addAllowedRolesToTokens(
    address[] calldata assetTokens,
    string[] calldata roles
) public override onlyOwner {
    for (uint i = 0; i < assetTokens.length; i++) {
        for (uint j = 0; j < roles.length; j++) {
            tokenAllowedRoles[assetTokens[i]].push(
                keccak256(abi.encodePacked(roles[j]))
            );
        }
    }
    emit AddedRolesToTokens(assetTokens, roles);
}

SOLVED: The \client team solved the issue in commit 9a16372 by applying the recommendations.

It was identified that revert strings are used in the AssetToken, BaseAccessControlPausable and BaseAccessControlPausableUpgradeable contracts. Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. If the revert string uses strings to provide additional information about failures (e.g. require(amount <= 1e9 * 10 ** decimals(), "Limit tokens to 1 billion to prevent overflow");), but they are rather expensive, especially when it comes to deploying cost, and it is difficult to use dynamic information in them.

Code Location

Example usage of revert strings in the AssetToken contract:

src/AssetToken.sol

function mint(address to, uint256 amount) public override onlyOwner {
    compliance.authorizeTransfer(msg.sender, to, address(this), amount);
    require(totalSupply() == 0, "Not allowing additional minting");
    require(decimals() <= 18, "Max decimals for token is 18");
    require(amount <= 1e9 * 10 ** decimals(), "Limit tokens to 1 billion to prevent overflow");
    super._mint(to, amount);
    IRewardsManager(rewardsManager).signalAssetTokensMinted();
}

SOLVED: The \client team solved the issue in commit d2bbf3a by using custom errors instead of reverting strings.