MARBLEX security record

The changes made in commit [d4302c2d89369e99b154b4c3cee7f7cb727878f0](https://github.com/MarblexAudit/MBXToken-ERC20/commit/d4302c2d89369e99b154b4c3cee7f7cb727878f0) introduce a lack of access control on a critically privileged function: ```solidity function grantRole(bytes32 role, address account) public override(AccessControl, IAccessControl) { _beforeSetRole(role, account, true); super._grantRole(role, account); } ``` This introduction allows anyone to call the function `grantRole()` because the override calls `super._grantRole()` instead of `super.grantRole()`. Since the external `super.grantRole()` is where the access protection is located, this function can now be called by anyone. In turn, anyone can take on the `MINTER_ROLE` and `PAUSER_ROLE`.

The `update_handled_nonce()` function updates nonce values in the bridge contract. The calculates a `limit` as the sum of `lowerHandleNonce` and `NONCE_LOWER_SEARCH_LIMIT`, ensuring it doesn't exceed `upperHandleNonce`. It iterates from 0 to `limit` and checks if each nonce exists in `handleNoncesToBlockNumbers`. If a nonce is missing in `handleNoncesToBlockNumbers`, the iteration breaks, and `lowerHandleNonce` is updated to this missing nonce value. ```rust=408 inline fun update_handled_nonce(requestedNonce_: u256) acquires BridgeNonceInfo, HandledInfo { let bridgeNonceInfo = borrow_global_mut<BridgeNonceInfo>(@warp); if(requestedNonce_ > bridgeNonceInfo.upperHandleNonce) { bridgeNonceInfo.upperHandleNonce = requestedNonce_; }; let limit: u256 = bridgeNonceInfo.lowerHandleNonce + NONCE_LOWER_SEARCH_LIMIT; if(limit > bridgeNonceInfo.upperHandleNonce) { limit = bridgeNonceInfo.upperHandleNonce; }; let handledInfo = borrow_global_mut<HandledInfo>(@warp); let i:u256 = 0; while(i <= limit) { if(simple_map::contains_key(&handledInfo.handleNoncesToBlockNumbers, &i) == false) break; i = i + 1; }; bridgeNonceInfo.lowerHandleNonce = i; } ``` There is a potential issue with the logic that increments `lowerHandleNonce`. If a nonce within the range from `lowerHandleNonce` to `limit` is not found in `handleNoncesToBlockNumbers`, it indicates that a particular nonce was not processed. In such a case, `lowerHandleNonce` is set to this unprocessed nonce, and the function exits. This implementation could potentially allow bypassing certain checks if specific nonces are never processed (i.e., they don't exist in `handleNoncesToBlockNumbers`). For example, if a user's request for calling `handle_bridge_swap()` was never to be triggered, the `lowerHandleNonce` will never be increased, and therefore, the following checks in the `handle_bridge_swap()` will always succeed. ```rust=341 let bridgeNonceInfo = borrow_global_mut<BridgeNonceInfo>(@warp); assert!(bridgeNonceInfo.lowerHandleNonce <= requestedNonce_, ERR_HANDLE_BRIDGE_SWAP_REQUEST_NONCE_LOWER_THAN_LOWERHANDLE); ``` According to the chain implementation, the `requestedNonce_` started from `1` and therefore, the `lowerHandleNonce` will always return 0 and never increase.

The linked comments do not correctly reflecting the access control management ```rust=158 /// Permission management is managed by sha3_256 (b"MINTER_ROLE") in access_control. /// - sender : A signer object for permission checking. /// - coin : The withdrawn coin object. public fun burn(sender: &signer, coin: coin::Coin<MBX>) acquires CoinRoleStore { whenNotPaused(); // modifier let amount = coin::value<MBX>(&coin); coin::burn<MBX>(coin, get_burn_cap(sender)); event::emit(BurnEvent { sender: address_of(sender), amount }); } /// Set the frozen state of 0x1::coin to true.\ /// Permission management is managed by sha3_256 (b"BURNER_ROLE") in access_control. /// - sender : A signer object for permission checking. /// - account : The address of the target account to be frozen. public entry fun set_freeze(sender: &signer, account: address) acquires CoinRoleStore { coin::freeze_coin_store(account, get_freeze_cap(sender)); event::emit(FreezeEvent { account }); } ``` - L158: MINTER_ROLE should be BURNER_ROLE - L169: MINTER_ROLE should be FREEZER_ROLE

The default value for `bridgeConfig.allowedToChainId` is 0, and the account `@warp` may update this value via `set_allowed_chain_id`. However, there is no limitation on the given `allowedToChainId_` value usage or update, which means 0 can still be valid.

When validate the signatures for the `handle_bridge_swap` operation, the function will go through a list of signatures and calculate the overall power, there are several concern about the current implementation: 1. The initial value for the threshold is `DEFAULT_THRESHOLD_VALUE` which is 0, if the admin not setup the value, there is no signature needed for the cross-chain operations. 2. There is no limitation on the single validator's power, for example, if one validator is too powerful that its power is already above the threshold, which will raise the chance for the single point failure.

When adding/removing admin, the module `warp::admin` will emit events about the admin update information. However, when adding admin, the module will emit a message with `MEMBER_REMOVED` instead of `MEMBER_ADDED`, which will raise an inaccurate message.

In the current implementation, the fee bridge can be set as any value. There should be a bound that limits how high the fees can be set in the linked functions.

The fee address will collect fee while `request_bridge_swap`. However, the initial value is `@0x00`, and there is no validation in the `set_fee_address`, for example, `@0x0`, `@aptos_framework` or `aptos_token`. The invalid account may cause: 1. fee loss 2. DoS on `request_bridge_swap`, for example, if the fee address set to `@0x0`, the transaction will be reverted with the following issue: ``` │ 190 │ ╭ assert!( │ 191 │ │ new_address != @vm_reserved && new_address != @aptos_framework && new_address != @aptos_token, │ 192 │ │ error::invalid_argument(ECANNOT_RESERVED_ADDRESS) │ 193 │ │ ); │ │ ╰─────────^ Test was not expected to error, but it aborted with code 65541 originating in the module 0000000000000000000000000000000000000000000000000000000000000001::account rooted here ```

The following potential typo inside the contract can be fixed: - L208: `swarp` should be `swap`

The view functions in the above-linked code use `borrow_global_mut` to acquire the information that returns a mutable reference. However, it is unnecessary to return a mutable reference since the corresponding variables will not be changed inside the code. Additionally, Aptos compiler will insert `freeze` instruction to allow the mutable reference used in a context where an immutable reference is expected. Ref: https://aptos.dev/move/book/references#freeze-inference ```rust=430 #[view] public fun get_bridge_nonce_info(): BridgeNonceInfo acquires BridgeNonceInfo { let bridgeNonceInfo = borrow_global_mut<BridgeNonceInfo>(@warp); *bridgeNonceInfo } #[view] public fun get_bridge_handled_info(): HandledInfo acquires HandledInfo { let handledInfo = borrow_global_mut<HandledInfo>(@warp); *handledInfo } #[view] public fun get_reqest_info_all(creator_: address): SimpleMap<vector<u8>, RequestDetail> acquires RequestLog { to_simple_map(&borrow_global<RequestLog>(creator_).requestDetails) } #[view] public fun get_reqest_info_for_uuid(creator_: address, uuid_: vector<u8>): RequestDetail acquires RequestLog { *smart_table::borrow(&borrow_global_mut<RequestLog>(creator_).requestDetails, uuid_) } #[view] public fun get_operator_list() : vector<address> { let operatorList = operator::get_operators(); operatorList } #[view] public fun get_resource_signer_address() : address acquires ModuleData{ let resource_signer = get_resource_signer(); let resourceSignerAddress = signer::address_of(&resource_signer); resourceSignerAddress } #[view] public fun get_bridge_config(): BridgeConfig acquires BridgeConfig { let bridgeConfig = borrow_global_mut<BridgeConfig>(@warp); *bridgeConfig } #[view] public fun get_fee_amount(): u64 acquires BridgeConfig { let bridgeConfig = borrow_global_mut<BridgeConfig>(@warp); bridgeConfig.feeAmount } #[view] public fun get_fee_address(): address acquires BridgeConfig { let bridgeConfig = borrow_global_mut<BridgeConfig>(@warp); bridgeConfig.feeAddress } ```