Centrifuge v3

Description

The multicall() function implements batching functionality but lacks proper transaction payment handling that should accompany the batching operations. The function is marked as payable, indicating it can receive ETH, but it only handles batching start/end operations without corresponding payment operations.

The vulnerability manifests in the edge case where:

1. A user calls multicall() with msg.value > 0 and an empty data array
2. The function starts batching, calls super.multicall(data) with no operations, and ends batching
3. No payment handling occurs, leaving the sent ETH permanently locked in the contract

While most individual functions in the contract are not payable and would revert if called with ETH, the empty data array scenario bypasses this protection since no function calls are attempted.

Note: Hub::multicall() and VaultRouter::multicall() do have functionallity to process msg.value.

Recommendation

Condider checking msg.value == 0 or checking data.length >0 in the BalanceSheet::multicall() function.

Description

The linkVault() and unlinkVault() functions in the Spoke contract access vault details directly without validating that the vault was properly deployed through the system. This inconsistency creates a potential avenue for linking unregistered vaults.

In the updateVault() function, there is an explicit check to ensure only validated vaults are processed:

// Needed as safeguard against non-validated vaults// I.e. we only accept vaults that have been deployed by the pool managerrequire(_vaultDetails[vault].asset != address(0), UnknownVault());

However, the linkVault() and unlinkVault() functions directly access _vaultDetails[vault] without this validation:

function linkVault(PoolId poolId, ShareClassId scId, AssetId assetId, IVault vault) public auth {    // ... other checks ...    VaultDetails storage vaultDetails_ = _vaultDetails[vault]; // Missing validation    require(!vaultDetails_.isLinked, AlreadyLinkedVault());    // ...}

This could allow administrators to link vaults that haven't been properly deployed through the deployVault() function, potentially leading to inconsistent system state.

Recommendation

Consider moving the vault validity check inside linkVault() / unlinkVault().

Description

The Hub contract's reentrancy protection mechanism using the _protected() internal function does not provide actual protection against reentrancy attacks. This could leave functions vulnerable to reentrancy, potentially allowing attackers to manipulate contract state or drain funds.

The Hub contract implements a _protected() internal function intended to prevent reentrancy:

/// @dev Protect against reentrancyfunction _protected() internal protected {}

This function is used in critical operations like notifyDeposit() and notifyRedeem() with the expectation that it will prevent reentrancy attacks. However, this implementation is ineffective because:

• The protected modifier's logic is not properly applied when used in this manner, because it is only on one location in the function. For nonreentrancy protection, these should be something at the beginning and the end of the function.

Functions relying on this protection include:

• notifyDeposit() - handles deposit notifications and callbacks
• notifyRedeem() - handles redemption notifications and callbacks

We don't see a real reentrancy possibility in these function, thus the risk is currently limited.

Note: a nonReentrancy mechanism is also important in relation to modifier payTransaction, because otherwise the transactionRefund can be overwritten.

The function _isManager() uses a similar pattern as _protected() and thus also doesn't protect against reentrancy. This is less important because its authorisation mechanism is effective.

Proof of Concept

// SPDX-License-Identifier: MITpragma solidity ^0.8.28;import "hardhat/console.sol";  contract testReentrancy {     error UnauthorizedSender();    address private transient _initiator;    modifier protected() {        if (_initiator == address(0)) {            _initiator = msg.sender;            _;            _initiator = address(0);        } else {            require(msg.sender == _initiator, UnauthorizedSender());            _;        }    }    function _protected() internal protected {}    function notifyDeposit(uint n) public {        console.log(n);        _protected();        if (n > 0) this.notifyDeposit(n-1);    }    function test() public {        notifyDeposit(5);    }}

Recommendation

Use the protected modifier instead of the _protected() function and remove the _protected() function.

For _isManager(): doublecheck if the intention of the protected modifier is to provide reentrancy protection. If so move if to the calling functions.

Otherwise consider removing the protected modifier from _isManager() because it doesn't add any value.

Centrifuge

Will be solved in a future release.

Description

The _verifyMainnetAddresses() function in FullDeployer.s.sol contains hardcoded address verification that does not match the actual deployed contract addresses documented in the official Centrifuge protocol deployments. Specifically, three contracts have mismatched addresses:

• asyncRequestManager: Script expects 0x58d57896EBbF000c293327ADf33689D0a7Fd3d9A but documentation shows 0xF06f89a1b6C601235729A689595571B7455dD433
• asyncVaultFactory: Script expects 0xE01Ce2e604CCe985A06FA4F4bCD17f1F08417BF3 but documentation shows 0xED9D489BB79c7cB58C522f36fC6944eaA95ce385
• syncDepositVaultFactory: Script expects 0x3568184784E8ACCaacF51A7F710a3DE0144E4f29 but documentation shows 0x21bf2544b5a0B03C8566a16592Ba1B3b192b50Bc

This discrepancy stems from version differences between v3.0.0 and v3.0.1 deployments, where the salt generation method changed from using hashed versions (keccak256(abi.encodePacked("3"))) to unhashed versions (bytes32(bytes("3"))).

Recommendation

Update the hardcoded addresses in _verifyMainnetAddresses() to match the correct v3.0.1 deployment addresses:

function _verifyMainnetAddresses() internal view {    // ... other addresses remain the same ...-   require(address(asyncRequestManager) == 0x58d57896EBbF000c293327ADf33689D0a7Fd3d9A);+   require(address(asyncRequestManager) == 0xF06f89a1b6C601235729A689595571B7455dD433);    require(address(syncManager) == 0x0D82d9fa76CFCd6F4cc59F053b2458665C6CE773);-   require(address(asyncVaultFactory) == 0xE01Ce2e604CCe985A06FA4F4bCD17f1F08417BF3);+   require(address(asyncVaultFactory) == 0xED9D489BB79c7cB58C522f36fC6944eaA95ce385);-   require(address(syncDepositVaultFactory) == 0x3568184784E8ACCaacF51A7F710a3DE0144E4f29);+   require(address(syncDepositVaultFactory) == 0x21bf2544b5a0B03C8566a16592Ba1B3b192b50Bc);    // ... rest of addresses remain the same ...}

Also update the deployment documentation.

Description

In several locations a check is done that address(subsidy[poolId].refund!=address(0). However in function _send() this isn't explicitly checked.

Recommendation

Consider using a general address if the refund address is zero. For example the GLOBAL_POT; although the requires additional administration. Alternatively remove rundunant zero checks.

Centrifuge

Acknowledged. Right now from a system point of view, it's impossible to reach a state where refund is 0, because for sending some pool-related message, the pool has requires to be configured, which means a call to setRefundAddress().

From an unitary point of view of the Gateway, I think this check no longer makes sense because using address(0) or another undesired address has the same issue for the caller: they can not recover they funds.

The check in subsidizePool needs to stay there because anyone could subsidize an inexistent pool. Nevertheless, I think we can remove the current check in _requestPoolFunding, given the above precondition.

Description

Function _requestPoolFunding() can retrieve funds from any contract that rely()s on the Gateway contract.

This can be done if that address is added via setRefundAddress().

The following contracts rely()s on the Gateway contract:

• multiAdapter
• messageProcessor

The risk is limited because setRefundAddress() is authorized, and multiAdapter and messageProcessor don't inherit Recoverable.

A special case is the situation where refund == address(this), then subsidy[GLOBAL_POT].value would be set to 0. This currently can't happen because Gateway doesn't rely() on itself.

Recommendation

Consider checking none of the system addresses are used in setRefundAddress(), especially address(this).

Centrifuge

Acknowledged. This is prevented by the authorization mechanism.

Description

poolId shouldn't be 0, because that is a special value that is used for GLOBAL_POT.

There is no explicit checked in:

• guardian::createPool()
• hub::createPool()
• hubRegistry.registerPool()

Note: there is an implicit check in hub::createPool() where it is checked with localCentrifugeId().

Note: the risk is limited because the function setting the poolId are authorized.

Recommendation

Consider explicitly checking poolId != 0.

Centifuge

Acknowledged. poolId contains a non-zero centrifugeId, see finding CentrifugeId shouldn't be 0.

Description

CentrifugeId should not be 0 because this is a special value. It is also not allowed in MessageProcessor::handle().

There is no explicit check that the CentrifugeId isn't 0, neither in the deployment script nor in the constructors of MultiAdapter or MessageDispatcher.

Recommendation

Consider explicitly checking CentrifugeId != 0 to prevent configuration mistakes.

Centrifuge

Acknowledged, this is handled by us as deployers.

Description

The OnOfframpManager constructor contains inconsistent variable naming that could lead to confusion for developers and auditors. The constructor parameter is named spoke_ but is assigned to the contractUpdater state variable. This naming mismatch is inconsistent with the OnOfframpManagerFactory, which correctly uses contractUpdater_ as the parameter name.

Additionally, the error message NotSpoke() in the update function is outdated and should use a more generic authorization error to match the naming convention used in similar contracts like MerkleProofManager.

Recommendation

Update the constructor parameter name and error message for consistency.

Description

Most interface files are changed to GPL-2.0-or-later in PR 477.

However the following are not:

• ITokenFactory.sol
• IVaultFactory.sol
• IPoolEscrowFactory.sol

Recommendation

Review and update license info.

Description

The messageGasLimit() function in the GasService contract uses an unnecessarily complex chain of else if statements. Since each condition branch contains a return statement, the else keywords are redundant and can be removed to improve code readability and maintainability.

The current implementation uses a long chain of else if statements:

if (kind == MessageType.ScheduleUpgrade) {    return scheduleUpgrade;} else if (kind == MessageType.CancelUpgrade) {    return cancelUpgrade;} else if (kind == MessageType.RecoverTokens) {    return recoverTokens;}// ... continues for all message types

Recommendation

Consider simplifying the conditional structure by removing the else keywords since each branch returns early:

if (kind == MessageType.ScheduleUpgrade) return scheduleUpgrade;if (kind == MessageType.CancelUpgrade)   return cancelUpgrade;if (kind == MessageType.RecoverTokens)   return recoverTokens;if (kind == MessageType.RegisterAsset)   return registerAsset;// ... apply same pattern to all remaining conditions

Description

The TokenRecoverer contract is deployed in the _preDeployCommon() function but is not included in the contract registration block. While the contract is properly deployed and integrated into the system architecture, it is missing from the registration process that records deployed contract addresses as well as the deployment documentation.

Recommendation

Add the TokenRecoverer contract to the registration block to ensure consistent documentation and tracking:

+ register("tokenRecoverer", address(tokenRecoverer));

Also add it to the deployment documentation.

Description

When net deposits == 0 then isIncrease is set to True. This is inconsistent with submitQueuedShares which uses shareQueue.isPositive which currently is false when the delta is 0.

Recommendation

Stay consistent by setting isIncrease to true when deposits > 0

Description

If a refund address would be shared among different pools, then the approach of _requestPoolFunding() is suboptimal, because it gets all funds from refund and uses it to subsidize poolId.

This would prevent other pools from using it, even though perhaps less funds are required.

Recommendation

Consider documenting at setRefundAddress() that refund addresses should not be shared between pools.

If sharing refund addresses would be relevant then consider retrieving only the required amount (eg. cost - subsidy[poolId].value ).

Description

uint96() truncates the parameter, without error. This is very unlikely to cause issues though because such large amounts of native tokens won't occur.

Recommendation

Consider using SafeCast.

Description

As found by the project: right now the vault deployments are not using create2, but the factories themselves are deployed deterministically. This means the vault addresses are based on the nonce, which means that across chains, different share tokens can have the same vault address.

This can lead to user confusion.

Recommendation

Consider using create2 and a salt to deploy the vaults.

Description

Most errors use custom errors however, some errors use strings. This is inconsistent.

Recommendation

Consider using custom errors everywhere.

Description

Two different patterns are used for parameters of file():

• wireWormholeAdapter uses twice file(..., centrifugeId, wormholeId, ...)
• wireAxelarAdapter uses file(..., axelarId, centrifugeId, ...) + file(.., centrifugeId, axelarId, ...)

The first pattern is easier because it only needs one file() function.

Recommendation

Consider using the first pattern also in wireAxelarAdapter.

Description

Function _safeGetAssetDecimals() used abi.encodeWithSignature(). However abi.encodeCall() could also be used, which has additional checks.

Recommendation

Consider using abi.encodeCall().

Description

There are some typos present.

Recommendation

Consider making the following changes:

• GasService: take -> taken

Description

The _verifyAdmin() check isn't foolproof. A vault contract could comply with these checks, while these signers would not be able to control the safe.

This could be done in the following ways:

• have a 10 of 20 multisig where an attacker controls the other 12 accounts
• have a guard that prevents just signing by these addresses
• have module that allows alternative ways to allow transactions
• or a fake contract that always return true on a call to isOwner()

See here for more info: https://ackee.xyz/blog/staying-safe-with-safe

Recommendation

Only use this check as a sanity check.

Centrifuge

Acknowledged. This is a check for us to double-check at deployment stage that the correct safe account is the configured one.

Description

timestampedPath contains block.chainid twice. According the rest of the string, the second instance should be block.number.

Recommendation

Consider replacing the second instance of block.chainid with block.number.

Description

In the deployment scripts, a large number of rely() statements are done. If accidentally an address is used for a contract that isn't deployed yet, then this isn't detected.

Note: the same issue is present with root::endorse().

Recommendation

Consider having a wrapper function in the deployment scripts that check user != address(0) for rely() and endorse().

Alternatively this check can also be added in the functions themselves.

Centrifuge

Acknowledged. This is already checked in tests.

Description

CommonDeployer has a comment that register("adminSafe"...) isn't necessary.

However load_vars.sh has been refactored into load_config.py, which doesn't do the register().

Recommendation

Consider adding register("adminSafe"...).

Centrifuge

We only register in register() contracts that we deploy to later be able to read them from the json. The adminSafe is already in the json (in another section). I've removed the comment which seems to no longer be correct and creates more confusion.

Description

Function maxDeposit() does an explicit type conversion from uint128 to uint256, because _maxDeposit() returns an uint128.

However the similar function maxRedeem() doesn't do this.

Recommendation

Consider also adding an explicit type conversion in function maxRedeem(). Alternatively remove the explicit conversion in maxDeposit().

Description

Small gas savings available by conditionally updating shareQueue.

Recommendation

Consider implementing something like:

-          shareQueue.isPositive = shareQueue.delta != 0;+          if (shareQueue.delta == 0 && shareQueue.isPositive) shareQueue.isPositive = false;

Centrifuge

Acknowledged. We prefer to keep the current version that reduces ifs.

Description

Once repay() succeeds, then the underpaid_[] isn't necessary any more. Keeping them increases the contract state.

Recommendation

Consider deleting underpaid[centrifugeId][batchHash] when underpaid_.counter reaches zero and the message is sucessfully send.

Description

Function newManager() checks the token isn't address(0).

However Spoke::shareToken() already checks the token !=0, so this check is redundant.

Recommendation

Consider removing the redundant check from newManager().