Plural Protocol Contracts (updates) - Plural Energy

The changeDepositories function alters the accounts designated for storing user deposits. If deposits exist in an old account, these deposits will be "hidden" from user balances once the depository is changed. This can lead to discrepancies in user balances, causing confusion and potential loss of funds visibility.

Following tests confirm this issue:

    function test_ReturnDeposit_ToHolder_DeprecatedDepository(address _newDepository, address _holder, uint256 _amount) public {
        vm.assume(_holder != address(0));
        vm.assume(_holder != _depository);
        vm.assume(_newDepository != _depository);
        vm.assume(_amount != 0);

        test_TakeDeposit_Working(_holder, _amount);

        vm.prank(_manager);
        assetToken.changeDepositories(toDyn([_newDepository]));
        assertEq(assetToken.balanceOf(_holder), uint256(0));
        assertEq(assetToken.ownedBalanceOf(_holder), _amount);
        assertEq(assetToken.balanceOf(_depository), _amount);
        assertEq(assetToken.ownedBalanceOf(_depository), uint256(0));
        assertEq(assetToken.balanceOf(_newDepository), 0);
        assertEq(assetToken.ownedBalanceOf(_newDepository), uint256(0));

        vm.prank(_depository);
        assetToken.returnDeposit(_holder, _holder, _amount);
        assertEq(assetToken.depositFrom(_holder), uint256(0));
        assertEq(assetToken.depositTo(_depository), uint256(0));
    }

    function test_ReturnDeposit_ToHolder_NewDepository(address _newDepository, address _holder, uint256 _amount) public {
        vm.assume(_holder != address(0));
        vm.assume(_holder != _depository);
        vm.assume(_newDepository != _depository);
        vm.assume(_amount != 0);

        test_TakeDeposit_Working(_holder, _amount);

        vm.prank(_manager);
        assetToken.changeDepositories(toDyn([_newDepository]));
        assertEq(assetToken.balanceOf(_holder), uint256(0));
        assertEq(assetToken.ownedBalanceOf(_holder), _amount);
        assertEq(assetToken.balanceOf(_depository), _amount);
        assertEq(assetToken.ownedBalanceOf(_depository), uint256(0));
        assertEq(assetToken.balanceOf(_newDepository), 0);
        assertEq(assetToken.ownedBalanceOf(_newDepository), uint256(0));

        vm.prank(_newDepository);
        assetToken.returnDeposit(_holder, _holder, _amount);
        assertEq(assetToken.depositFrom(_holder), uint256(0));
        assertEq(assetToken.depositTo(_depository), uint256(0));
    }

With following results:

To prevent this, it's crucial to implement a mechanism to transfer existing deposits to the new depository or properly account for them during the transition, ensuring accurate balance tracking and user trust.

Remediation Plan

SOLVED: The Plural Energy team added convenient checks and events to prevent changing depositories until they are empty.

The codebase contains TODOs and developer notes, indicating incomplete features or areas requiring further attention. These placeholders suggest that the contract may not be fully implemented or tested, posing potential risks for deployment.

Addressing these TODOs and reviewing the notes is crucial to ensure the contract's functionality, security, and readiness for production. Proper documentation and resolution of these items are essential to maintain code quality and reliability.

Remediation Plan

SOLVED: The Plural Energy team has removed the comment from the codebase.

The contract overrides the _upgradeTo function from UUPSUpgradeable to add the onlyOwner modifier, but it also overrides _authorizeUpgrade with the same purpose. This results in redundant code, as both functions are essentially performing the same access control check.

To streamline the code and improve maintainability, it is recommended to use only _authorizeUpgrade for the ownership check and remove the redundancy in _upgradeTo. This approach maintains the necessary security while reducing code duplication and potential errors.

Remediation Plan

SOLVED: The Plural Energy team proceed to delete _upgradeTo overrided function.

The contract lacks validation checks for zero amounts in its functions. Failing to verify that amounts are non-zero can lead to unintended behavior, such as unnecessary transactions or logic execution. Implementing zero checks ensures that only meaningful and valid transactions are processed, enhancing contract efficiency, security, and reliability. This preventive measure reduces potential errors and maintains the integrity of the contract’s operations.

To address this issue, add checks to ensure amounts are greater than zero before proceeding with function logic.

Example implementation:

function transfer(address recipient, uint256 amount) public returns (bool) {
    require(amount > 0, "Amount must be greater than zero");
    // existing transfer logic
}

By incorporating such checks, the contract ensures that only valid, non-zero transactions are processed, improving overall robustness and preventing unnecessary operations.

Remediation Plan

SOLVED: The Plural Energy team added convenient checks to their codebase.

The contract AssetToken does not emit events during critical state changes in takeDeposit nor returnDeposit, resulting in a lack of transparency and traceability.

Event emissions are crucial for monitoring contract activity, debugging, and providing real-time updates to users and external systems. Implementing event emissions ensures that state changes are recorded on the blockchain, enabling better auditing and enhancing user trust.

To address this issue, include event emissions in functions where state changes occur.

Example implementation:

event StateChanged(address indexed user, uint256 newValue);

function changeState(uint256 newValue) public {
    // existing state change logic
    emit StateChanged(msg.sender, newValue);
}

By emitting events during state changes, the contract provides clear and transparent logs of its activities, improving overall accountability and reliability.

Remediation Plan

SOLVED: The Plural Energy team added convenient events to their codebase.

The returnDeposit function can modify depositFrom and depositTo using the unchecked keyword because these values are already validated to prevent underflow.

Leveraging unchecked in this context can optimize gas usage by avoiding redundant checks, given that the necessary validations are already in place.

Example implementation demonstrating the safe use of unchecked:

    function returnDeposit(
        address holder,
        address tokenRecipient,
        uint amount
    ) public override onlyDepository {
        // Validate
        if (holder == address(0)) revert ZeroAddress();
        if (tokenRecipient == address(0)) revert ZeroAddress();
        if (depositFrom[holder] < amount) revert InsufficientDeposit(holder);
        if (depositTo[msg.sender] < amount)
            revert InsufficientDeposit(msg.sender);

        ........
        unchecked {
            depositFrom[holder] -= amount;
            depositTo[msg.sender] -= amount;
        }
        _transfer(tokenRecipient, amount, false);
    }

By implementing unchecked in the returnDeposit function after proper validation, the contract enhances efficiency without compromising security.

Remediation Plan

ACKNOWLEDGED: The Plural Energy team acknowledged this finding.

Methods _transfer and _transferFrom are using a condition to check the outcome of OpenZeppelin's ERC20 functions, which revert on failure instead of returning a boolean. This can lead to incorrect error handling and unexpected behavior. OpenZeppelin's ERC20 functions use revert to signal failures, making conditional checks ineffective for these cases. Proper error handling should be implemented to catch these reverts and handle them accordingly.

Error handling by using try/catch is a solution, but just receiving the revert coming from the ERC20, could be enough.

Remediation Plan

SOLVED: The Plural Energy team decided to relay in OpenZeppelin's ERC20 revert behavior so deleted the conditionals from their codebase.