Photon Messaging Protocol (EVM) - Entangle Labs

The MSCProposeHelper contract has a vulnerability in the proposeUpdateTransmitters function that allows unauthorized access to update the transmitters for a specified protocol on a specified chain. The function is marked as external, which means it can be called by any external account without any access control restrictions. This allows an attacker to propose arbitrary changes to the transmitters, potentially adding or removing transmitters without proper authorization.

    /// @notice Make proposal to update allowed transmitter to specified protocol on specified chain
    /// @param protocolId protocol id
    /// @param chainId target chain id
    /// @param toAdd transmitter array of evm addresses to add
    /// @param toRemove transmitter array of evm addresses to remove
    function proposeUpdateTransmitters(
        bytes32 protocolId,
        uint256 chainId,
        address[] memory toAdd,
        address[] memory toRemove
    ) external {
        bytes memory selector = PhotonFunctionSelectorLib.encodeEvmSelector(
            0x654b46e1
        ); // EndPointGov.updateTransmitters(bytes)
        GovMessages.UpdateTransmittersMsg memory message = GovMessages.UpdateTransmittersMsg(
            protocolId,
            toAdd,
            toRemove
        );
        bytes memory params = abi.encode(message);
        endPoint.propose(govProtocolId, chainId, govContractAddresses[chainId], selector, params);
    }

// SPDX-License-Identifier: MIT
pragma solidity 0.8.19;

import "forge-std/Test.sol";
import "./MSCProposeHelper.sol";

contract MSCProposeHelperTest is Test {
    MSCProposeHelper public mscProposeHelper;
    bytes32 public constant protocolId = bytes32("test-protocol");
    uint256 public constant chainId = 1;

    function setUp() public {
        // Deploy the MSCProposeHelper contract
        mscProposeHelper = new MSCProposeHelper();
        
        // Initialize the contract with dummy addresses
        address[] memory initAddresses = new address[](3);
        initAddresses[0] = address(0x1); // Admin
        initAddresses[1] = address(0x2); // MasterSmartContract
        initAddresses[2] = address(0x3); // EndPoint
        mscProposeHelper.initialize(initAddresses);
    }

    function testProposeUpdateTransmitters() public {
        address[] memory toAdd = new address[](2);
        toAdd[0] = address(0x4);
        toAdd[1] = address(0x5);
        
        address[] memory toRemove = new address[](1);
        toRemove[0] = address(0x6);
        
        // Call the proposeUpdateTransmitters function directly
        mscProposeHelper.proposeUpdateTransmitters(
            protocolId,
            chainId,
            toAdd,
            toRemove
        );
        
        // Assert that the proposal was made successfully
        // (additional assertions can be added based on the specific implementation)
    }
}

To address this vulnerability, it is crucial to implement proper access control mechanisms for the proposeUpdateTransmitters function.

Remediation Plan

SOLVED : The Entangle team solved the issue by implementing the onlyMsc modifier.

The MSCProposeHelper contract has a vulnerability in the proposeRemoveTransmitters function that allows unauthorized access to remove transmitters from a specified protocol on a specified chain. The function is marked as external, which means it can be called by any external account without any access control restrictions. This allows an attacker to propose the removal of transmitters without proper authorization, potentially disrupting the protocol's functioning and compromising its integrity.

    /// @notice Make proposal to removing allowed transmitter to specified protocol on specified chain
    /// @param protocolId protocol id
    /// @param chainId target chain id
    /// @param transmitters transmitter array of evm address to remove
    function proposeRemoveTransmitters(
        bytes32 protocolId,
        uint256 chainId,
        address[] memory transmitters
    ) external {
        bytes memory selector = PhotonFunctionSelectorLib.encodeEvmSelector(
            0x5206da70
        ); // EndPointGov.removeTransmitters(bytes)
        GovMessages.AddOrRemoveTransmittersMsg memory message = GovMessages
            .AddOrRemoveTransmittersMsg(protocolId, transmitters);
        bytes memory params = abi.encode(message);
        endPoint.propose(govProtocolId, chainId, govContractAddresses[chainId], selector, params);
    }

// SPDX-License-Identifier: MIT
pragma solidity 0.8.19;

import "forge-std/Test.sol";
import "./MSCProposeHelper.sol";

contract MSCProposeHelperTest is Test {
    MSCProposeHelper public mscProposeHelper;
    bytes32 public constant protocolId = bytes32("test-protocol");
    uint256 public constant chainId = 1;

    function setUp() public {
        // Deploy the MSCProposeHelper contract
        mscProposeHelper = new MSCProposeHelper();
        
        // Initialize the contract with dummy addresses
        address[] memory initAddresses = new address[](3);
        initAddresses[0] = address(0x1); // Admin
        initAddresses[1] = address(0x2); // MasterSmartContract
        initAddresses[2] = address(0x3); // EndPoint
        mscProposeHelper.initialize(initAddresses);
    }

    function testProposeRemoveTransmitters() public {
        address[] memory transmittersToRemove = new address[](2);
        transmittersToRemove[0] = address(0x4);
        transmittersToRemove[1] = address(0x5);
        
        // Call the proposeRemoveTransmitters function directly
        mscProposeHelper.proposeRemoveTransmitters(
            protocolId,
            chainId,
            transmittersToRemove
        );
        
        // Assert that the proposal was made successfully
        // (additional assertions can be added based on the specific implementation)
    }
}

To mitigate this vulnerability, it is essential to implement proper access control mechanisms for the proposeRemoveTransmitters function.

Remediation Plan

SOLVED : The Entangle team solved the issue by implementing onlyMsc modifier.

The selectAgentsTransmitters function in the StakingManager contract is responsible for selecting agent (transmitter) addresses for a given protocol. However, there is a missing check to ensure that the selected agents are not paused. This can lead to the selection of paused agents as transmitters, which may cause issues in the protocol's functionality and reliability.

In the current implementation, the function checks various conditions for an agent to be selected as a transmitter, such as protocol support, minimum personal stake, and minimum total delegation. However, it does not verify if the agent is currently paused using the agentManager.pausedAgents(agentAddr) function.

    /// @notice Select agent (transmitter) addresses for protocol
    /// @param protocolId - protocol id
    /// @param agents - max amount of transmitters to select
    function selectAgentsTransmitters(
        bytes32 protocolId,
        uint agents,
        address[] memory cachedAgents
    ) internal view returns (address[] memory) {
        uint minPersonalStake = externalDeveloperHub.minPersonalAmount(protocolId);
        uint minTotalDelegation = externalDeveloperHub.minDelegateAmount(protocolId);
        address[] memory selected = new address[](agents);
        uint n;
        uint i;
        while (i < cachedAgents.length && n < agents) {
            address agentAddr = cachedAgents[i];
            //require(agentAddr != address(0), "Test: agent address should not be 0");
            AgentInfo storage agent = agentInfo[agentAddr];
            if (
                agentManager.protocolSupported(agentAddr, protocolId) &&
                agentsGlobal.contains(agentAddr) &&
                agent.activeRoundStake >= minTotalDelegation &&
                agent.personalStake >= minPersonalStake
            ) {
                address transmitter = agentManager.transmitters(agentAddr, protocolId);
                /*require(
                    transmitter != address(0),
                    "Test: transmitter should always exist at that point"
                );*/
                selected[n] = transmitter;
                unchecked {
                    ++n;
                }
            }
            unchecked {
                ++i;
            }
        }
        assembly {
            mstore(selected, n)
        }
        return selected;
    }

To address this issue, it is recommended to add a check for paused agents within the selectAgentsTransmitters function. The agentManager.pausedAgents(agentAddr) function should be used to verify if an agent is paused before selecting them as a transmitter.

Remediation Plan

SOLVED : The Entangle team solved the issue by implementing the suggested recommendation.

The contracts are upgradable, inheriting from the Initializable contract. However, the current implementation is missing the _disableInitializers() function call in the constructor. Thus, an attacker can initialize the implementation.

Usually, the initialized implementation has no direct impact on the proxy itself; however, it can be exploited in a phishing attack. In rare cases, the implementation might be mutable and may have an impact on the proxy.

It is recommended to call the function _disableInitializers() within the contract's constructor to prevent the implementation from being initialized.

Remediation Plan

RISK ACCEPTED: The Entangle team accepted the risk of the issue.

The PhotonFunctionSelectorLib library provides functionality for encoding and decoding function selectors, including support for Solana function selectors. However, there are a few areas that could be improved to ensure the library's compatibility and reliability when working with Solana selectors.

1. Absence of Specific Functions for Solana Selectors: - The library does not provide dedicated functions for encoding or decoding Solana selectors separately. - Developers using the library need to manually specify the appropriate selectorType and selector values when calling the encodeFunctionSelector and decodeFunctionSelector functions. - This lack of specific functions for Solana selectors may lead to potential misuse or errors if the wrong selectorType or selector values are provided.

2. Insufficient Testing and Examples for Solana Selectors: - The provided code does not include any test cases or examples specifically demonstrating the encoding and decoding of Solana selectors. - Without comprehensive testing and examples, it is difficult to ensure that the library functions correctly and reliably when handling Solana selectors. - The absence of test cases and examples may lead to potential bugs or compatibility issues going unnoticed.

3. Assumption of Solana Selector Compatibility: - The library assumes that Solana selectors (both anchor and native) can be represented as bytes and fall within the SELECTOR_MAX_LEN limit. - However, there is no explicit validation or checks to ensure that the provided Solana selectors meet these requirements. - If a Solana selector exceeds the maximum length or cannot be properly encoded as bytes, it may lead to unexpected behavior or errors.

To address the identified issues and enhance the usability and reliability of the PhotonFunctionSelectorLib library for Solana function selectors, the following recommendations are proposed:

1. Implement Specific Functions for Solana Selectors: - Add dedicated functions for encoding and decoding Solana selectors, such as encodeSolanaAnchorSelector, encodeSolanaNativeSelector, decodeSolanaAnchorSelector, and decodeSolanaNativeSelector. - These functions should handle the specific requirements and formats of Solana selectors, abstracting away the need for manual selectorType and selector value assignment.

Remediation Plan

RISK ACCEPTED : The Entangle team accepted the risk of the issue.

The reentrancy risk arises from the fact that the function performs a low-level call to the protocol contract using protocolAddress.call{value: msg.value}(...) without any reentrancy protection. If the called contract is malicious and attempts to re-enter the executeOperation function before the state changes are persisted, it can lead to unintended consequences.

    /// @notice execute approved operation
    /// @param opData 1
    /// @param transmitterSigs 2
    function executeOperation(
        OperationLib.OperationData calldata opData,
        Signature[] calldata transmitterSigs
    ) external payable isAllowedExecutor(opData.protocolAddr) {
        bytes32 msgHash = keccak256(
            abi.encodePacked(
                opData.protocolId,
                opData.meta,
                opData.srcChainId,
                opData.srcBlockNumber,
                opData.srcOpTxId,
                opData.nonce,
                opData.destChainId,
                opData.protocolAddr,
                opData.functionSelector,
                opData.params,
                opData.reserved
            )
        );
        bytes32 opHashBytes = keccak256(
            abi.encodePacked("\x19Ethereum Signed Message:\n32", msgHash)
        );
        uint256 opHash = uint256(opHashBytes);

        if (opData.destChainId != __chainId()) {
            revert Endpoint__OpIsNotForThisChain(opHash);
        }

        if (opExecutedMap[opHash]) {
            revert Endpoint__OpIsAlreadyExecuted(opHash);
        }

        address protocolAddress = abi.decode(opData.protocolAddr, (address));
        bytes32 protocolId = protocolAddressToProtocolId[protocolAddress];

        if (protocolId != opData.protocolId || allowedProtocolInfo[opData.protocolId].isCreated == false) {
            revert Endpoint__InvalidProtocolId(opData.protocolId);
        }

        address[] memory uniqSigners = new address[](transmitterSigs.length);
        uint uniqSignersCnt;
        bool consensusReached;
        uint numOfAllowedTransmitters = numberOfAllowedTransmitters[protocolId];
        if (numOfAllowedTransmitters == 0) {
            revert Endpoint__ZeroTransmittersCount(protocolId);
        }
        uint k;
        uint consensusTargetRate = allowedProtocolInfo[protocolId].consensusTargetRate;
        for (uint i; i < transmitterSigs.length; ) {
            address signer = ecrecover(
                opHashBytes,
                transmitterSigs[i].v,
                transmitterSigs[i].r,
                transmitterSigs[i].s
            );
            if (signer != address(0) && allowedTransmitters[protocolId][signer]) {
                bool isNewSigner = true;
                delete k;
                while (k < uniqSignersCnt) {
                    if (uniqSigners[k] == signer) {
                        isNewSigner = false;
                        break;
                    }
                    unchecked {
                        ++k;
                    }
                }
                if (isNewSigner) {
                    uniqSigners[uniqSignersCnt] = signer;
                    ++uniqSignersCnt;

                    uint256 consensusRate = (uniqSignersCnt * rateDecimals) /
                        numOfAllowedTransmitters;
                    if (consensusRate >= consensusTargetRate) {
                        consensusReached = true;
                        break;
                    }
                }
            }
            unchecked {
                ++i;
            }
        }

        if (consensusReached) {
            (uint8 selectorType, bytes memory selectorDecoded) = PhotonFunctionSelectorLib.decodeFunctionSelector(
                opData.functionSelector
            );
            assert(selectorType == uint8(PhotonFunctionSelectorLib.SelectorTypes.EVM));
            bytes4 selector = abi.decode(selectorDecoded, (bytes4));
            (bool success, bytes memory ret) = protocolAddress.call{value: msg.value}(
                abi.encodeWithSelector(
                    selector,
                    abi.encode(
                        opData.protocolId,
                        opData.srcChainId,
                        opData.srcBlockNumber,
                        opData.srcOpTxId,
                        opData.params
                    )
                )
            );

            if (success) {
                opExecutedMap[opHash] = true;
            }
            // TODO: remove before mainnet
            else {
                revert Endpoint__TargetCallError(ret);
            }
            emit ProposalExecuted(opHash, success, ret);
        } else {
            revert Endpoint__OpIsNotApproved(opHash);
        }
    }

It is recommended to implement re-entrancy protection on the codebase.

Remediation Plan

ACKNOWLEDGED : The Entangle team acknowledged the issue.

The current ownership transfer process for all the contracts inheriting from Ownable or OwnableUpgradeable involves the current owner calling the [transferOwnership()](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.8/contracts/access/Ownable.sol#L69-L72) function:

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

If the nominated EOA account is not a valid account, it is entirely possible that the owner may accidentally transfer ownership to an uncontrolled account, losing the access to all functions with the onlyOwner modifier.

Consider using Ownable2Step instead of Ownable contract.

Remediation Plan

SOLVED : The Entangle team solved the issue by using admin role instead of owner.

The EndPointGov contract inherits from both OwnableUpgradeable and AccessControlUpgradeable. However, the contract does not utilize the owner variable and associated functionality provided by the OwnableUpgradeable contract. This redundancy can lead to confusion and unnecessary complexity in the contract's access control mechanism.

The OwnableUpgradeable contract provides an owner variable and related functions such as transferOwnership, renounceOwnership, and modifiers like onlyOwner. These features are not being used in the EndPointGov contract, as the access control is primarily managed through the roles defined in the AccessControlUpgradeable contract.

To address this issue, it is recommended to remove the OwnableUpgradeable inheritance from the EndPointGov contract. Since the contract already utilizes the AccessControlUpgradeable contract for access control, the OwnableUpgradeable inheritance can be safely removed.

Remediation Plan

SOLVED : The Entangle team solved the issue by removing the redundant variable.

The executeOperation function is responsible for executing an approved operation on the destination chain. However, the function includes a loop that iterates over the transmitterSigs array to verify the signatures and check if consensus is reached among the transmitters.

If the number of transmitter signatures (transmitterSigs.length) is very large, the loop could consume a significant amount of gas, potentially leading to an out-of-gas exception. This is because the loop performs ecrecover operations and other computations for each signature, which can be gas-intensive.

Impose a reasonable limit on the number of transmitter signatures allowed in the executeOperation function. This limit should be based on the expected maximum number of transmitters and the gas limits of the network.

Remediation Plan

ACKNOWLEDGED : The Entangle team acknowledged the issue.

In the updateTransmitters function of the MasterSmartContract, there is an open TODO comment that highlights the need to ensure sufficient old transmitters are available for consensus when adding new governance (gov) transmitters. The function is responsible for updating the list of transmitters for a specified protocol based on the new transmitters array provided.

Currently, the function calculates the transmitters to be added (toAdd) and removed (toRemove) based on the differences between the current transmitters and the new transmitters array. It then broadcasts the changes to the relevant chains, where the protocol is initialized.

However, the TODO comment suggests that there might be a potential issue when adding new governance transmitters. If a significant number of old transmitters are removed and replaced with new governance transmitters, it could potentially impact the consensus process.

To address the open TODO and ensure the stability and security of the protocol when adding governance transmitters, the following recommendations should be considered:

1. Implement a Gradual Transition:

   • Introduce a gradual transition process when adding new governance transmitters.

   • Retain a sufficient number of old transmitters to maintain consensus stability during the transition.

   • Gradually introduce the new governance transmitters while ensuring a smooth transition.

Remediation Plan

ACKNOWLEDGED : The Entangle team acknowledged the issue.

During the review of the smart contract code in MasterSmartContract.sol and EndPoint.sol, several open TODO comments were identified. These comments indicate areas where additional functionality or improvements are needed to ensure the security, stability, and proper functioning of the contracts. Leaving these TODOs unaddressed can potentially lead to vulnerabilities, unexpected behavior, or incomplete implementation.

./MasterSmartContract.sol:656:        // TODO: ensure sufficient old transmitters for consensus when adding gov transmitters
./MasterSmartContract.sol:947:                // TODO: if new roud detected, we need to release bets for old round proofs and remove them
./EndPoint.sol:499:            // TODO: remove before mainnet

Conduct a comprehensive review of the entire codebase to identify any additional TO-DOs or areas that require attention. Prioritize addressing these TO-DOs based on their potential impact on the system's security, stability, and functionality.

Remediation Plan

SOLVED : The Entangle team solved the issue by resolving TO-DOs.