6.1 Deposits have no depositor-side bound on fees, so a concurrent fee update can silently consume part of the deposit
Whitelisted depositors lock BNB by calling the deposit(...) function of the CBNBRouter contract. They send the BNB as msg.value, and the router deducts two fees whose values are read from storage at execution time: the fixed cryptoFee and the proportional protocolFeeBps. The router forwards the remainder to the vault. The relayer mints exactly this remainder as cBNB on Canton. The fees stay in the router as feeBalance, which only the admin can collect.
function deposit(bytes calldata _recipient)
// ...
{
// ...
if (msg.value <= cryptoFee) {
revert InvalidCryptoFeeAmount();
}
// @audit-issue The fee values are read at execution time, and the depositor cannot bound them.
uint256 depositAmount = msg.value - cryptoFee;
uint256 protocolFee = _calculateFee(depositAmount);
uint256 amountAfterFees = depositAmount - protocolFee;
// @audit The only check on the net amount is the protocol-controlled minimum.
if (amountAfterFees < minAmount) {
revert AmountBelowMinimum();
}
uint256 totalFees = cryptoFee + protocolFee;
if (totalFees > 0) {
feeBalance += totalFees;
}
messageId = vault.deposit{value: amountAfterFees}(/* ... */);
}
The Manager Safe or the Admin Safe can change both fees at any time through the setFees(...) function. The change takes effect immediately, with no delay. Only protocolFeeBps has an upper bound (10%); cryptoFee has none.
function setFees(uint256 _newCryptoFee, uint16 _newProtocolFeeBps)
external
onlyManagerOrAdmin
{
if (_newProtocolFeeBps > MAX_PROTOCOL_FEE_BPS) {
revert FeeTooHigh();
}
// @audit-issue There is no upper bound on the fixed fee and no delay before it applies.
cryptoFee = _newCryptoFee;
protocolFeeBps = _newProtocolFeeBps;
emit FeesUpdated(_newCryptoFee, _newProtocolFeeBps);
}
The deposit(...) function does not let the depositor state the fees they agreed to or the minimum amount they expect to have bridged. The only check on the net amount is minAmount, and the protocol sets it, not the depositor. If a fee update is executed before a pending deposit, whether in the same block or while the deposit is waiting to be mined, the deposit still succeeds. It is charged the new fees as long as the remainder stays at or above minAmount. This needs no malicious intent: any routine fee change, such as the first move away from the current 0 / 0 configuration, applies to every deposit that was signed under the old fees and is mined after the update.
Recommendation
Consider giving depositors a way to make sure that a deposit is only charged the fees they agreed to, for example by binding each deposit to an expected fee or a minimum bridged amount. If the deployed contracts are to stay as they are, consider an operational procedure that keeps fee updates from overlapping with pending deposits (for example, pausing deposits, waiting until pending deposits are mined or dropped, and only then updating the fees), and communicating fee changes to whitelisted depositors before they apply. Also consider placing an upper bound on cryptoFee.
Resolution
Only Nuxaris-controlled addresses are whitelisted depositors, therefore this cannot affect external depositors.
6.2 Deposit message IDs are only unique within a single vault deployment
Whitelisted depositors bridge BNB to Canton by calling the deposit(...) function of the CBNBRouter contract, which forwards the net amount to the deposit(...) function of the CBNBVault contract. The vault derives a messageId for every deposit and emits it in the Deposit event. The off-chain relayer watches these events and, once the block is finalized, mints the same amount of cBNB on Canton. The NatSpec describes messageId as the identifier "used for idempotent processing", and the README states that it "is unique per deposit, because it includes the vault nonce".
/// @return messageId Unique message identifier used for idempotent processing
function deposit(...)
external
payable
onlyRouter
returns (bytes32 messageId)
{
// ...
messageId = _nextMessageId(_sender, msg.value, _dstChainID, _dstOutputToken, _recipient);
// @audit The relayer reads the messageId from this event.
emit Deposit(messageId, /* ... */);
}
The _nextMessageId(...) function computes the messageId as the hash of the depositor, the chain ID, the vault's nonce, the net amount, the destination and the recipient. The nonce is incremented on every deposit, so two deposits into the same vault always receive different IDs, even when all their other parameters are identical.
function _nextMessageId(
address _sender,
uint256 _amount,
uint256 _dstChainID,
bytes32 _dstOutputToken,
bytes memory _recipient
) internal returns (bytes32) {
// @audit-issue The nonce is vault storage and starts at 0 in every new vault.
uint256 currentNonce = nonce++;
// @audit-issue None of the hashed fields identifies the vault instance.
return keccak256(
abi.encodePacked(
_sender,
block.chainid,
currentNonce,
_amount,
_dstChainID,
_dstOutputToken,
_recipient
)
);
}
However, this uniqueness only holds within a single vault instance. The nonce is a storage variable of the vault and starts at 0 in every newly deployed vault, and none of the hashed fields identifies the vault itself. If the vault is redeployed on the same chain for any reason, its nonce does not continue from the previous vault's nonce. The k-th deposit into the new vault then receives exactly the same messageId as the k-th deposit into the old vault whenever both have the same depositor, net amount, destination and recipient. This breaks the uniqueness and idempotency guarantee of messageId, so any off-chain infrastructure or process that relies on it, such as the relayer, would be working on a wrong assumption.
Recommendation
Consider accounting for this limitation in any future vault deployment or migration, so that deposit message IDs remain unique across all vault instances.
Resolution
Only Nuxaris-controlled addresses are whitelisted depositors, therefore this cannot affect external depositors.
