XP Network - Dfinity Integration

The `withdraw_nft()` function, which is defined as an external function accessible by any account on the ICP chain, unlocks the source NFT which was locked on an external chain by burning wrapped NFT on the ICP chain. However, its implementation lacks verification codes for amount of NFT transaction fee and ownership of wrapped NFT, allowing malicious users to call the `withdraw_nft()` function with fake arguments and obtain the source NFT on the external chain for only the ICP transfer fee.

From a design perspective, the `FEEBLOCK_STORE` variable was created to prevent duplicate fee_block submissions. ```rust async fn require_tx_fee( canister_id: &Principal, caller: &Principal, fee_block: BlockIndex, ) -> Result<u64, BridgeError> { if FEEBLOCK_STORE.with(|store| store.borrow().contains(&fee_block)) { return Err(BridgeError::InvalidFee); } ... } ``` Since the function does not store `fee_block` into `FEEBLOCK_STORE`, `FEEBLOCK_STORE` is always empty, which means that this check is meaningless. If the fee oracle can guarantee that the incoming `fee_block` will never be repeated, then there is no problem. But we think the best way to deal with it is to store the verified fee_block in `FEEBLOCK_STORE`. In this way, every time a new `fee_block` comes in, the requirement can check whether it already exists in the `FEEBLOCK_STORE`, thus ensuring that the same `fee_block` is not submitted repeatedly.

In the execution of a transfer operation within the [Ledger Canister](https://github.com/dfinity/ledger-ref/blob/3ed0206afd231b9586c9e7b9a793948f10118297/src/Ledger.mo#L209), a fee must be paid, and it will check if the source account holds a sufficient amount of ICP to cover the transfer amount plus the fee. In the rust code, the logic is to withdraw all the ICP balance from the `minter` contract, and the `amount` passed to the `transfer` method is equal to the total ICP balance, with a standard fee of 10^-4 ICP. The sum of the `amount` and `fee` will exceed the amount of ICP held by the `minter` contract, resulting in the transfer failing. The actual transaction amount should be `bal- fee`.

There will be a potential infinite loop for method `clean_logs()` in the `minter` canister.\ In that method, there is no validation for input parameter `action`.\ If `action.from_action` is bigger than `action.to_action`, it will case the not intended looping at L401. ```rust=401 while action.from_action != action.to_action { bmap.remove(&action.from_action); action.from_action += Nat::from(1u32); } ```

The `unsafe` block in Line 58 is duplicate and could be replaced by calling function `config_ref()`. ```rust=58 let conf = unsafe { CONFIG.as_ref().unwrap() }; ```

Loop iterates over `action.token_urls[i]` vector (i=0... size of token_urls) and accesses `action.mint_with[i]` vector without asserting that `action.token_urls` and `action.mint_with` have equal lengths. ```rust=461 for (i, token_url) in action.token_urls.into_iter().enumerate() { xpnft_mint(action.mint_with[i], token_url, action.to) ``` If the length of `action.mint_with` is less than `action.token_urls`, a panic will be caused due to the subscript being out of bounds. The similar issue also exists in the function `validate_unfreeze_nft_batch()`.

The logic of `clean_logs()` function is to delete a specified `action` item. If the designated `action` number does not exist, the `clean_logs()` function will not throw an error, but it will waste Cycles. To avoid the pointless consumption of Cycles, it is necessary to check the existence of the `action` number.

It is unnecessary to use `clone` as this value is dropped without further use. ```rust=152 let key = PublicKey::new(config_ref().group_key.clone()); ```

The above line creates new references, which are immediately dereferenced by the compiler. ```rust=207 let caller_acc = AccountIdentifier::new(&caller, &DEFAULT_SUBACCOUNT); let canister_acc = AccountIdentifier::new(&canister_id, &DEFAULT_SUBACCOUNT); ``` However, since the `caller` and `canister_id` are already references of the `Principal` type, which meet the parameter type requirements of the function, there is no need to create new references again.