In the changeBurnerLimit and changeMintingLimit functions, which are executed via setBridgeLimits(), any limit set below 86400 (equal to 1 days in Solidity) will result in a zero rate per second, affecting the possibility of dynamic limit within the DURATION threshold. This may occur because the rate per second is calculated as following in the aforementioned functions:

bridges[_bridge].minterParams.ratePerSecond = _limit / _DURATION;

bridges[_bridge].burnerParams.ratePerSecond = _limit / _DURATION;

where uint256 private constant _DURATION = 1 days.

This may result in a static limit for the specified bridge, as with a rate per second with 0 value will not change the _calculatedLimit value in the following code execution whenever the _getCurrentLimit() function is called:

uint256 _timePassed = block.timestamp - _timestamp;
uint256 _calculatedLimit = _limit + (_timePassed * _ratePerSecond);
_limit = _calculatedLimit > _maxLimit ? _maxLimit : _calculatedLimit;

It is recommended to set a minimum threshold when invoking setBridgeLimits() or, alternatively, incorporate a validation step to confirm that ratePerSecond is not set to zero.

Remediation Plan

SOLVED: The Tagus Labs team has addressed the finding in commit a98d3e2 by following the mentioned recommendation of adding a check for a minimum threshold.

When using Solidity versions 0.8.0 and above, the compiler natively handles overflows and underflows by reverting the transaction if these issues occur. In the XERC20 contract, specifically in the _getCurrentLimit() function, there's a potential for an unhandled overflow, which could cause the transaction to revert when calculating the current limit:

uint256 _calculatedLimit = _limit + (_timePassed * _ratePerSecond);

If the multiplication operation between _timePassed and _ratePerSecond results in a value greater than type(uint256).max, the transaction will revert. Similarly, if the result of the addition _limit and (_timePassed * _ratePerSecond) is greater than type(uint256).max, the transaction will also revert, effectively denying service for the bridge attempting to mint or burn XERC20 tokens under these circumstances.

For example, even though the limit set is very high, a bridge is not able to mint 2 tokens due to the overflow occuring when calculating the new limit.

  function test_mintVeryHighLimitDOS(address _bridge, uint256 _burningLimit, uint256 amount) public {
    vm.assume(_burningLimit != 0);
    vm.assume(_bridge != address(0));
    amount = bound(amount, 1, type(uint256).max);
    test_setBridgeLimits(_bridge, type(uint256).max, _burningLimit);
    vm.startPrank(_bridge);

    uint256 currentLimitBefore = xerc20.mintingCurrentLimitOf(_bridge);
    uint256 maxLimitBefore = xerc20.mintingMaxLimitOf(_bridge);

    xerc20.mint(_bridge, 1);
    assertEq(xerc20.balanceOf(_bridge), 1);
    skip(1);
    vm.expectRevert();
    xerc20.mint(_bridge, 1);
  }

It is recommended to add a threshold in the setBridgeLimits() function to ensure that no calculated limit will exceed the maximum value of a uint256 variable, allowing bridges to mint and burn when needed.

Remediation Plan

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

This bug is next to impossible due to the range of values we’re working with in this case. In particular, we’re dealing with timestamps vs. uint256. It turns into impossible time limits.

Several contracts in scope have owners with privileged rights to perform administrative tasks and need to be trusted to not perform malicious updates.

The administrative functions enable the owner to add and remove bridges, adjust their limits, set caps and their durations, designate token destinations, configure contracts, and pause or unpause the bridge.

It is recommended to implement a role-based access control mechanism to allow multiple entities to perform admin tasks and limit powers within the system to users according to their roles, effectively reducing centralization risks.

Remediation Plan

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

We’re aware of that, and therefore we use a 3-5 multisig address as the owner.

The current implementation of the InceptionBridge contract employs a single-step ownership transfer mechanism, transferring ownership directly to another account in one transaction. This is achieved through the inheritance of OpenZeppelin's OwnableUpgradeable contract. Similarly, the XERC20 contract inherits from OpenZeppelin's Ownable contract.

This approach taken for both contracts mentioned does not align with best practices regarding security measures, as the address to which the ownership is transferred should be verified to be active or be willing to act as the owner.

It is recommended to use OpenZeppelin's Ownable2Step or Ownable2StepUpgradeble contracts over the Ownable and OwnableUpgradeable currently in use. Alternatively, consider implementing similar two-step ownership transfer logic into the contract.

Remediation Plan

RISK ACCEPTED: The Tagus Labs team made a business decision to accept the risk of this finding and not alter the contracts.

The _addDestination() function from the InceptionBridge contract has a check to verify that the _bridgeAddressByChainId mapping is properly set up and is not equivalent to address(0. However, it does not perform the same verification for the fromToken or the toToken input values.

This may incur in possible denial of service for addresses attempting to execute deposit() or withdraw().

Add a check to ensure that neither the fromToken and toToken addresses are address(0).

Remediation Plan

SOLVED: The Tagus Labs team has addressed the finding in commit a98d3e2 by following the mentioned recommendation.

Throughout the codebase, there are multiple instances where the deposit and withdraw functions do not check if the amount to deposit or withdraw is zero. This may result in unnecessary gas consumption or unexpected behavior.

The affected functions are:

- deposit and withdraw functions in the InceptionBridge contract.

- deposit, depositTo, depositNative, depositNativeTo, withdraw and withdrawTo functions in the XERC20Lockbox contract.

Add checks in the aforementioned functions to ensure that the amount to deposit or withdraw is greater than zero.

Remediation Plan

ACKNOWLEDGED: The Tagus Labs team made a business decision to acknowledge this finding and not alter the contracts.

Throughout the codebase, there are multiple functions that are marked as public but are not used internally in the contract they are declared. This may result in unnecessary gas consumption.

The affected functions are:

- mint, burn, setLockbox, mintingMaxLimitOf, burningMaxLimitOf in the XERC20 contract.

- depositNativeTo in the XERC20Lockbox contract.

- getDeploymentCreate2Address in the BridgeFactory contract.

Modify the aforementioned functions with the external visibility modifier.

Remediation Plan

SOLVED: The Tagus Labs team has addressed the finding in commit a98d3e2 by following the mentioned recommendation.

In the deposit function of the InceptionBridge contract, the post-increment operator is used to increment the _globalNonce variable:

_globalNonce++;

This is not the most gas efficient approach to incrementing a variable.

Replace the post-increment operator with the pre-increment operator to reduce gas costs. Additionally, the piece of code can be executed inside an unchecked block, since realistically it is not possible for the _globalNonce to overflow in practice:

unchecked{
  ++_globalNonce;
}

Remediation Plan

SOLVED: The Tagus Labs team has addressed the finding in commit a98d3e2 by following the mentioned recommendation.

In the InceptionBridge.sol file, OpenZeppelin's IERC20Metada.sol is imported but not used in the contract.

Remove the unused import statement.

Remediation Plan

SOLVED: The Tagus Labs team has addressed the finding in commit a98d3e2 by removing the unused import file.

The InitializableTransparentUpgradeableProxy contract contains require statements that are missing error descriptions, particularly in the initialize() function:

require(_implementation() == address(0));

and the _requireZeroValue() function:

require(msg.value == 0);

Use descriptive reason strings. Alternatively, it is recommended the use of custom errors to follow the project's error standardization, best practices and to improve gas efficiency.

Remediation Plan

SOLVED: The Tagus Labs team has addressed the finding in commit a98d3e2 by using descriptive reason strings.

To ensure stability, it's important that contracts are 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

ACKNOWLEDGED: The Tagus Labs team made a business decision to acknowledge this finding and not alter the contracts.

The compiler for Solidity 0.8.20 switches the default target EVM version to Shanghai, which means that the generated bytecode will include PUSH0 opcodes. Be sure to select the appropriate EVM version in case you intend to deploy on a chain apart from mainnet like L2 chains that may not support PUSH0, otherwise deployment of your contracts will fail.

Make sure to specify the target EVM version when using Solidity 0.8.20 and above, especially if deploying to L2 chains that may not support the PUSH0 opcode. Stay informed about the opcode support of different chains to ensure smooth deployment and compatibility.

Remediation Plan

ACKNOWLEDGED: The Tagus Labs team made a business decision to acknowledge this finding and not alter the contracts.

The bridge contract operates under the assumption that all bridgeable tokens across supported chains have uniform decimal places. When tokens are deposited, the system records the quantity of tokens. Conversely, during withdrawals, it is assumed that the exact figure is used to mint tokens for users. If the referenced tokens have different decimal scaling, it can lead to discrepancies between these values.

On the other hand, it is understood that since the bridgeable tokens are managed internally, it is expected that they are all issued with the same number of decimal places, which would make it unlikely to impact the current codebase.

It is recommended to document this assumption clearly in the system specifications to mitigate any potential errors eventually arising from decimal inconsistencies in future upgrades.

Remediation Plan

ACKNOWLEDGED: The Tagus Labs team made a business decision to acknowledge this finding and not alter the contracts.

Throughout the codebase, there are multiple instances where full file imports are used. While this does not affect functionality, it is not considered a best practice.

Use named import syntax instead of importing full files. This restricts what is being imported to just the named items, not everything in the file, for example:

import {MyContract} from "src/MyContract.sol"

Remediation Plan

ACKNOWLEDGED: The Tagus Labs team made a business decision to acknowledge this finding and not alter the contracts.