RouterFacet Manual Review Findings

RFT-01M: Improper Service Fee Enforcement

Type	Severity	Location
Logical Fault		RouterFacet.sol:L78

Description:

The comparison utilized for guaranteeing the feeOwed is satisfied is open-ended, permitting values that exceed it which can occur due to accidental misuse by users.

Example:

Copy
77uint256 feeOwed = serviceFee();
78require(msg.value >= feeOwed, "Router: insufficient fee");

Recommendation:

If the functionality to overpay is desired i.e. for prioritization purposes, we advise this to be clearly depicted in the codebase. Alternatively, we advise the linked comparison to be set to an equality one instead.

Alleviation:

The Alliance Block team stated that overpayment is indeed meant to be kept by them in case of direct contract use as the front-end supplies the correct values. We strongly advise equality to be imposed as the fees can change between a transaction's submission on the network and execution, a blockchain trait that cannot be prevented by correct front-end values.

RFT-02M: Inexistent Prevention of Re-Invocation

Type	Severity	Location
Standard Conformity		RouterFacet.sol:L15-L25

Description:

The linked function is meant to be invoked as an initialization by the diamond cut pattern, however, it contains no access control.

Example:

Copy
15/**
16 * @notice initializes the state for the Router facet
17 * @param data_ abi encoded data - chain ID, teleport address and dAppId.
18 */
19function init(bytes memory data_) external {
20    LibRouter.Storage storage rs = LibRouter.routerStorage();
21    (rs.chainId, rs.teleportAddress, rs.dAppId) = abi.decode(data_,
22        (uint8, address, bytes32));
23
24    emit TeleportAddressSet(rs.teleportAddress);
25}

Recommendation:

Even though it may not be directly exposed by the diamond instance, it is still good practice to explicitly prevent initialization functions from being re-invoked. We advise this to be done so by ensuring that rs.chainId is zero when init is invoked.

Alleviation:

The Alliance Block team renamed the function from init to state, illustrating that the functions could potentially be re-invoked in the future as part of a state reset. As a result, we consider this exhibit adequately dealt with.

RFT-03M: Potentially Empty Name & Symbol

Type	Severity	Location
Standard Conformity		RouterFacet.sol:L92, L112

Description:

The bridge implementation permits a _lockMint operation on an NFT that possesses no known name and symbol, a trait we strongly advise against as it can lead to ambiguous NFT implementations whose wrapped implementation deviates only in the address.

Example:

Copy
86/**
87 *  @param collectionAddress_ The ERC721 contract address
88 *  @param tokenIds_ An array of token IDs to be locked and bridged
89 *  @return Payload for sending native tokens to a non-native chain
90 *  @dev If the ERC721 contract supports the metadata extension, we have to pass it too
91 */
92function _lockMint(address collectionAddress_, uint256[] calldata tokenIds_) internal returns (bytes memory) {
93    string[] memory tokenURIs = new string[](tokenIds_.length);
94    string memory collectionName;
95    string memory collectionSymbol;
96
97    if (ERC721(collectionAddress_).supportsInterface(type(IERC721Metadata).interfaceId)) {
98        for (uint256 i = 0; i < tokenIds_.length; i++) {
99            tokenURIs[i] = ERC721(collectionAddress_).tokenURI(tokenIds_[i]);
100            ERC721(collectionAddress_).transferFrom(msg.sender, address(this), tokenIds_[i]);
101        }
102
103        collectionName = string(abi.encodePacked("Wrapped ", ERC721(collectionAddress_).name()));
104        collectionSymbol = string(abi.encodePacked("W", ERC721(collectionAddress_).symbol()));
105    }
106    else {
107        for (uint256 i = 0; i < tokenIds_.length; i++) {
108            ERC721(collectionAddress_).transferFrom(msg.sender, address(this), tokenIds_[i]);
109        }
110    }
111
112    return abi.encode(tokenIds_, LibRouter.routerStorage().chainId, tokenURIs, collectionName, collectionSymbol);
113}

Recommendation:

We recommend revising the management of non-metadata compliant EIP-721 tokens by either not supporting them at all or providing them with a specialized wrapper name that includes their native chain address.

Alleviation:

The Alliance Block team stated that contracts not conforming to the metadata type are seldom and as such, they have opted to a uniform approach sacrificing clarity.