Taiko

In the case of recalling a canonical token present on chain A that user wanted to send on different chain (Chain B) , The bridge calls onMessageRecalled() which checks that if (bridgedToCanonical[ctoken.addr].addr != address(0)) is true then mints the token in the if{} block. if false then transfers the token(s). So in this case it will check the value of ctoken address in the bridgedToCanonical mapping. It would be 0 so the if condition will fail and the else{} will be executed as intended which will transfer the previously sent token back to message.owner.But in the case where user recalls the message which was about sending the bridged token from the chain B to chain A the same onMessageRecalled() function gets executed on the vault by Bridge contract, Now this time it checks the same condition i.e if(bridgedToCanonical[ctoken.addr].addr != address(0)) then mints, otherwise transfers the token to the message.owner . But because this time it’s trying to recall the bridged token that got burned while sending and the ctoken (i.e ctoken.addr) in this case would be canonical token address, which is getting encoded in _handleMessage() on src chain vault while sending the token. So checking if(bridgedToCanonical[ctoken.addr].addr != address(0)) on src chain (the chain where bridged token is deployed in this case) where message is getting recalled will fail because theres no token mapped for bridgedToCanonical[canonical token address]. (The same issue is present in ERC20Vault, ERC721Vault, ERC1155Vault)

Whenever user calls ERC20Vault::sendToken passing along msg.value for the fee which is collected if all execution goes through successfully including the transfer of the actual funds to be sent performed in ERC20Vault::_handleMessage(), HoweverERC20Vault::_handleMessage() ignores return value by t.transferFrom({from:msg.sender,to:address(this),amount:amount}). This means whenever transfer fails and returns false, execution won't fail and return but still take user msg.value (for fee) and sends the message for _balanceChange=0.

In the _invokeMessageCall(), there is a low level external call to the address to specified along with encoded data, it is assumed not to be an issue if originated from the vault contracts, however, it is also intended that message can also be sent directly with the bridge contract in which the to is specified by the user same as the data

In the onMessageRecalled of the ERC20Vault which is used to retrieve a user's fund back on source chain if status failed, the data which is encoded from the vault contract in the format: (CanonicalERC20 token, address user, address to, uint256 amount), is decoded back to get the amount and the token that was being sent, however this decoding is done incorrectly in the sense that the address that was taken to be the token is actually the address of the user as can be seen from the snippet, the actual token is the first value with the type CanonicalERC20 after the function selector.

receiveToken, onMessageRecalled and _handleMessage functions in the contract use a for loop to call safeTransferFrom and mint methods to transfer and mint tokens.Instead of using safeTransferFrom methods in the loop, safeBatchTransferFrom can be used to transfer the batch of tokens. And instead of using mint() on the bridged token every time in the loop, the _mintBatch() can be used from the OZ ERC1155 implementation which can be exposed publicly to ERC1155Vault as it's happening in the case of mint(). This should be noted that _mintBatch() and safeBatchTransferFrom() will also use the loops.

While setting an address the old address and new address can be same. To avoid spending unnecessary gas and emitting a new event for this, having a check before assignment will be a best practice.

processMessage allows anyone to execute the transactions on the bridge. There can be multiple possibilities where a user will try to use the transaction initiated from bridge (by call() ) to make a call on the arbitrary addresses.One example of it is a malicious user will try to steal the approvals given to the bridge where a malicious user will execute the transaction on message.to (token address) and will call transferFrom() to transfer it to the arbitrary address. The action of sending a transaction by bridge can be limited to only certain functions depending on the intended logic similarly what is happening in the recallMessage() for IRecallableSender(message.from).onMessageRecalled call.

Using single step ownership transfer is not secure, the existing owner can assign any new address as owner, and lose the ownership if the assigned address is incorrect address which can’t perform actions.Ownable2StepUpgradable provides added safety where the pending owner can accept the ownership due to its two-step process to transfer the ownership.

Anyone can execute the the transaction on any arbitrary address with processMessage it is possible that the message.to can be the address used by protocol in the future which only checks that the caller/msg.sender is Bridge contract address for authorization logic, in this case it is possible for anyone to execute the transaction on the in-protocol contract address using processMessage().

Bridge contract contains an unused error B_INVALID_SIGNAL and two unused events SignalSent, DestChainEnabled.