The _update() function in the GorplesCoin contract performs the calculation of tokens to burn prior to updating the balances of the involved addresses.

However, the calculation of the fees to burn is not executed when either of the two addresses involved is excluded from the burn mechanism. This results in the burn not being applied even if only the recipient (to address) is excluded from the burn, while the intention seems to be to burn tokens from the from address.

function _update(address from, address to, uint256 amount) internal override {
if (blacklisted[from] || blacklisted[to]) revert BorpaToken__E11();

    if (!isExcludedFromBurn[from] && !isExcludedFromBurn[to]) {
      uint256 fees = (amount * BURN_PERCENTAGE) / BURN_DENOMINATOR;
      if (fees > 0) {
        amount -= fees;
        totalSystemBurnedAmount = totalSystemBurnedAmount + fees;
        super._update(from, address(0), fees);
      }
    }
    super._update(from, to, amount);
}

In the following scenario, the recipients of the minted tokens are excluded from burning and the fee is not calculated for the sender:

function test_recipientExcludedFromBurn(uint _mintAmount, uint _fee, uint256 _burnAmount) public {
  _mintAmount = bound(_mintAmount, 1, type(uint128).max);
  _fee = bound(_fee, 0, _mintAmount);
  setupContract();

  //Exclude alice from burning
  vm.startPrank(ADMIN);
  address[] memory addresses = new address[](2);
  bool[] memory status = new bool[](2);
  addresses[0] = alice;
  addresses[1] = feeCollector;
  status[0] = true;
  status[1] = true;
  borpaToken.setBurnExcluded(addresses, status);
  vm.stopPrank();

  bytes32 testBytes = keccak256("bytes32Value");
  bytes memory recipientInBytes = abi.encode(alice);

  bytes memory parameters = abi.encode(recipientInBytes, _mintAmount, _fee, [testBytes, testBytes], 0, testBytes);
  bytes memory b = abi.encode(testBytes, 0, 0, [testBytes, testBytes], parameters);
  vm.startPrank(address(endpoint));
  borpaToken.redeem(b);

  assertEq(borpaToken.balanceOf(alice), _mintAmount - _fee);
  assertEq(borpaToken.balanceOf(feeCollector), _fee);
}

To ensure that the burn fee is applied correctly based on the from address alone, the logic should be modified to check and exclude fee calculation only if the from address is excluded from the burn mechanism, e.g.,:

if (!isExcludedFromBurn[from]) {
  ...
}

Remediation Plan

RISK ACCEPTED: The Entangle team has made a business decision to accept the risk of this finding and not alter the contracts, stating that:

It was designed to take fee only on the path, where both from and to are NOT excluded. So, if 1 of these addresses is excluded (e.g. one of our smart contracts) - fee will not be taken even if the excluded address is "to". You can imagine approve+deposit situation for this example.

In the GorplesCoin contract, there are several state-changing functions that remain accessible even when the contract is paused, as they lack the whenNotPaused() modifier.

These functions allow for tokens to be minted, approved, transferred or burned while the contract is paused, which can lead to several issues, such as inconsistent state management, off-chain monitoring discrepancies and potential security risks, as the paused state is typically intended to halt all operations that can modify the contract’s state, providing a safe window to address issues.

The affected functions are:

• claimMasterRewards()

• burn()

• systemBurn()

• approve()

• transfer()

• transferFrom()

It is recommended to verify the paused state of the contract across all crucial state-changing functions by applying the whenNotPaused modifier. This ensures that these operations are halted when the contract is paused, maintaining the integrity and security of the contract.

Remediation Plan

RISK ACCEPTED: The Entangle team has made a business decision to accept the risk of this finding and not alter the contracts, stating that:

Pause/unpause functionality was designed specifically for the bridge functions to not have transactions "stuck" in between networks during updates of agent transmitters network of Photon CCM. When bridge is paused we want Borpa ecosystem to continue functioning.

The GorplesCoin contract inherits from OpenZeppelin's PausableUpgradeable contract and implements the whenNotPaused modifier in the redeem() and bridge() functions. However, the contract does not include an external method to enable the pause mechanism to temporarily halt the contract's functionality.

A pause mechanism is essential for emergency situations, such as discovering a critical bug or vulnerability, and can prevent further damage to the contract and its users. Without a pause mechanism, the contract may be vulnerable to exploitation or unintended behavior in case of an emergency, and the whenNotPaused modifier will have no effect, as the paused state is unreachable with current implementation.

Add an external method to enable the pause mechanism. This method should only be accessible by authorized addresses.

Remediation Plan

SOLVED: The Entangle team has addressed the finding in commit fe8eb2b by following the mentioned recommendation.

From Solidity versions 0.8.20 through 0.8.24, the default target EVM version is set to Shanghai, which results in the generation of bytecode that includes PUSH0 opcodes. Starting with version 0.8.25, the default EVM version shifts to Cancun, introducing new opcodes for transient storage, TSTORE and TLOAD.

In this aspect, it is crucial to select the appropriate EVM version when it's intended to deploy the contracts on networks other than the Ethereum mainnet, which may not support these opcodes. Failure to do so could lead to unsuccessful contract deployments or transaction execution issues, particularly over cross-chain functionality like the codebase in scope.

Make sure to specify the target EVM version when using Solidity versions from 0.8.20 and above if deploying to chains that may not support newly introduced opcodes. Additionally, it is crucial to stay informed about the opcode support of different chains to ensure smooth deployment and compatibility.

Remediation Plan

PARTIALLY SOLVED: The Entangle team has partially solved the issue by setting an explicit Solidity compiler version to, ⁣ but0.8.24 lacks a specific EVM version for compilation and deployment. Further testing is required to ensure cross-chain compatibility.

In the GorplesCoin contract, the redeem() function is susceptible to a replay attack, where the txHash parameter could potentially be used multiple times to execute the function with the same data.

This txHash parameter, as decoded from the b calldata, is used as part of the bridging transaction confirmation without any mechanism to ensure its uniqueness or to prevent its reuse. As a result, the contract may be susceptible to a replay attack, where the same txHash could potentially be used to execute the redeem() function multiple times, leading to unauthorized or unintended transactions.

function redeem(bytes calldata b) external onlyRole(ENDPOINT) whenNotPaused {
    (, , , , bytes memory params) = abi.decode(b, (bytes32, uint256, uint256, bytes32[2], bytes));
    (bytes memory _to, uint256 _amount, uint256 _fee, bytes32[2] memory _txhash, uint256 _fromChain, bytes32 _marker) = abi.decode(
        params,
        (bytes, uint256, uint256, bytes32[2], uint256, bytes32)
    );
    address to = abi.decode(_to, (address));
    _mint(to, _amount - _fee);
    _mint(feeCollector, _fee);
    emit BridgeDone(to, _amount, _txhash, _fromChain, _marker);
}

It is worth mentioning that the ENDPOINT contract that executes the redeem() function will verify if the hash of the complete OperationData has been executed previously, as referenced in the following out-of-scope contract: https://github.com/Entangle-Protocol/photon-cross-chain-messaging/blob/develop/contracts/EndPoint.sol#L421.

In the following scenario, the same txHash is used twice to execute the same function call:

function test_redeemTxReplayAttack(uint amount, uint fee) public {
  setupContract();
  vm.startPrank(address(endpoint));
  bytes32 testBytes = keccak256("bytes32Value");
  bytes memory recipientInBytes = abi.encode(alice);

  amount = bound(amount, 1, type(uint128).max);
  fee = bound(fee, 0, amount);
  uint amountMinusFee = amount - fee;

  bytes memory parameters = abi.encode(recipientInBytes, amount, fee, [testBytes, testBytes], 0, testBytes);
  bytes memory b = abi.encode(testBytes, 0, 0, [testBytes, testBytes], parameters);

  borpaToken.redeem(b);
  borpaToken.redeem(b);
}

Introduce additional validation in the redeem() function to verify that each txHash has not been used in previous transactions.

Remediation Plan

RISK ACCEPTED: The Entangle team has made a business decision to accept the risk of this finding and not alter the contracts, stating that:

Protected by MSC (Master Smart Contract in Photon CCM) and EndPoint contract, which calls Borpa on destination chain.

Throughout the GorplesCoin contract, there are several instances of unoptimized for loop declarations that may incur in higher gas costs than necessary. The affected functions are:

• setMinBridgeAmount()

• setBlacklisted()

• setBurnExcluded()

• pendingEmission()

Optimize the for loop declarations to reduce gas costs. Best practices include: the non-redundant initialization of the iterator with a default value (i=0 instead of simply i), the use of the pre-increment operator (++i), and the unchecked keyword for incrementing by 1. For example:

for (uint256 i; i < loopAmount;) {
  // code logic
  unchecked { ++i; }
}

Additionally, it can be considered to remove unneeded declaration of local variables inside loops. Particularly, in the setMinBridgeAmount() function:

uint256 key = chainIds[i];
uint256 value = minAmounts[i];
minBridgeAmounts[key] = value;

Here, the key and value variables can be omitted if the intention is to use these values only once:

minBridgeAmounts[chainIds[i]] = minAmounts[i];

Remediation Plan

SOLVED: The Entangle team has addressed the finding in commit c116124 by following the mentioned recommendation.

Throughout the GorplesCoin contract, there are several instances where administrative functions change contract state by modifying core state variables. However, these changes are not reflected in any event emission.

These functions include:

• setBorpaMaster()

• setMinBridgeAmount()

• setBlacklisted()

• setBurnExcluded()

• setEmissionStart()

• setChestManager()

• setEndPoint()

• setBridgeRouterAddress()

• setFeeCollector()

Introduce event declarations and ensure their emission within the aforementioned functions. This change will enhance transparency and facilitate accurate off-chain monitoring.

Remediation Plan

ACKNOWLEDGED: The Entangle team made a business decision to acknowledge this finding and not alter the contracts, stating:

Not sure if we need that since all functions are meant to be invoked once, and every address of setting is public by default (or will be after fixes).

In the GorplesCoin contract, some of the state variables declared are not explicitly declared with any visibility modifiers. In Solidity, the compiler will assume any state variable declared in the contract as internal by default. However, it is considered best practice to explicitly specify visibility to enhance clarity and prevent ambiguity. Clearly labeling the visibility of all variables and functions will help in maintaining clear and understandable code.

The affected variables are: protocolId, eobChainId, endPoint and minBridgeAmounts.

Explicitly define the visibility for each state variable within the contract.

Remediation Plan

SOLVED: The Entangle team has addressed the finding in commit efdbc68 by following the mentioned recommendation.

In the setFeeCollector() function, the if (_feeCollector == address(0)) revert(); statement is missing an error description in case of not meeting the requirements.

Providing clear error descriptions helps developers and users quickly identify issues when transactions do not proceed as expected.

Add an error description to the aforementioned statement.

Remediation Plan

ACKNOWLEDGED: The Entangle team made a business decision to acknowledge this finding and not alter the contracts, stating that:

The function is only called by admin with one possible error.

The file in scope currently use floating pragma version ^0.8.20, which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.20, and less than 0.9.0.

However, it is recommended that contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

Lock the pragma version to the same version used during development and testing.

Remediation Plan

SOLVED: The Entangle team has addressed the finding in commit 077dbcc by following the mentioned recommendation.

In Solidity, it is a common practice to name internal functions with a single underscore prefix. This is to indicate that these functions are not meant to be used outside of the contract. However, the emitAllocations() function in the GorplesCoin contract does not follow this naming convention.

Additionally, the startRecepient variable from the BorpaToken contract appears to be misnamed. While this typo does not affect the functionality of the contract, it can lead to confusion when reading through the codebase.

To adhere to best practices and improve code readability, it is recommended to rename the internal function emitAllocations() to _emitAllocations(). Additionally, rename the startRecepient variable to startRecipient.

Remediation Plan

SOLVED: The Entangle team has addressed the finding in commit a16656a by following the mentioned recommendation.

While the majority of the functions in the GorplesCoin have NATSPEC comments in functions, there are still some functions and state variables that are missing these comments, which may their readability and the overall user experience. Proper NATSPEC documentation is crucial for enhancing the clarity and usability of smart contracts, especially for developers and end-users interacting with them.

Add proper NATSPEC documentation to the entirety of the contract.

Remediation Plan

SOLVED: The Entangle team has addressed the finding in commit 4388085 by following the mentioned recommendation.

The GorplesCoin contract currently defines the currentEmissionRate() function with public visibility, even though it is not called from within the smart contract, resulting in higher gas costs than necessary.

Modify the aforementioned function with the external visibility modifier.

Remediation Plan

SOLVED: The Entangle team has addressed the finding in commit f0bac73 by following the mentioned recommendation.