remaining comments

contracts/InterchainTokenService.sol

@@ -825,7 +825,6 @@ contract InterchainTokenService is
      * @param params Additional parameters for the token manager deployment.
      */
     function _deployTokenManager(bytes32 tokenId, TokenManagerType tokenManagerType, bytes memory params) internal {
-        // slither-disable-next-line controlled-delegatecall
         (bool success, bytes memory returnData) = tokenManagerDeployer.delegatecall(
             abi.encodeWithSelector(ITokenManagerDeployer.deployTokenManager.selector, tokenId, tokenManagerType, params)
         );
@@ -836,8 +835,9 @@ contract InterchainTokenService is
             tokenManager_ := mload(add(returnData, 0x20))
         }
 
-        if (tokenManagerType == TokenManagerType.LOCK_UNLOCK || tokenManagerType == TokenManagerType.LOCK_UNLOCK_FEE)
+        if (tokenManagerType == TokenManagerType.LOCK_UNLOCK || tokenManagerType == TokenManagerType.LOCK_UNLOCK_FEE) {
             ITokenManager(tokenManager_).approveService();
+        }
 
         // slither-disable-next-line reentrancy-events
         emit TokenManagerDeployed(tokenId, tokenManager_, tokenManagerType, params);
@@ -872,7 +872,6 @@ contract InterchainTokenService is
         address distributor;
         if (bytes(distributorBytes).length != 0) distributor = distributorBytes.toAddress();
 
-        // slither-disable-next-line controlled-delegatecall
         (bool success, bytes memory returnData) = interchainTokenDeployer.delegatecall(
             abi.encodeWithSelector(
                 IInterchainTokenDeployer.deployInterchainToken.selector,
@@ -961,7 +960,6 @@ contract InterchainTokenService is
         uint256 tokenManagerType;
         (tokenManagerType, tokenAddress) = ITokenManagerProxy(tokenManager_).getImplementationTypeAndTokenAddress();
 
-        // slither-disable-next-line controlled-delegatecall
         (bool success, bytes memory data) = tokenHandler.delegatecall(
             abi.encodeWithSelector(ITokenHandler.takeToken.selector, tokenManagerType, tokenAddress, tokenManager_, from, amount)
         );
@@ -977,9 +975,9 @@ contract InterchainTokenService is
         address tokenManager_ = tokenManagerAddress(tokenId);
 
         (uint256 tokenManagerType, address tokenAddress) = ITokenManagerProxy(tokenManager_).getImplementationTypeAndTokenAddress();
+
         ITokenManager(tokenManager_).addFlowIn(amount);
 
-        // slither-disable-next-line controlled-delegatecall
         (bool success, bytes memory data) = tokenHandler.delegatecall(
             abi.encodeWithSelector(ITokenHandler.giveToken.selector, tokenManagerType, tokenAddress, tokenManager_, to, amount)
         );

contracts/TokenHandler.sol

@@ -40,14 +40,17 @@ contract TokenHandler is ITokenHandler, ITokenManagerType, ReentrancyGuard {
             _giveTokenLockUnlock(tokenAddress, tokenManager, to, amount);
             return amount;
         }
+
         if (tokenManagerType == uint256(TokenManagerType.LOCK_UNLOCK_FEE)) {
             amount = _giveTokenLockUnlockFee(tokenAddress, tokenManager, to, amount);
             return amount;
         }
+
         if (tokenManagerType == uint256(TokenManagerType.MINT_BURN) || tokenManagerType == uint256(TokenManagerType.MINT_BURN_FROM)) {
             _giveTokenMintBurn(tokenAddress, to, amount);
             return amount;
         }
+
         revert UnsupportedTokenManagerType(tokenManagerType);
     }
 
@@ -72,18 +75,22 @@ contract TokenHandler is ITokenHandler, ITokenManagerType, ReentrancyGuard {
             _takeTokenLockUnlock(tokenAddress, tokenManager, from, amount);
             return amount;
         }
+
         if (tokenManagerType == uint256(TokenManagerType.LOCK_UNLOCK_FEE)) {
             amount = _takeTokenLockUnlockFee(tokenAddress, tokenManager, from, amount);
             return amount;
         }
+
         if (tokenManagerType == uint256(TokenManagerType.MINT_BURN)) {
             _takeTokenMintBurn(tokenAddress, from, amount);
             return amount;
         }
+
         if (tokenManagerType == uint256(TokenManagerType.MINT_BURN_FROM)) {
             _takeTokenMintBurnFrom(tokenAddress, from, amount);
             return amount;
         }
+
         revert UnsupportedTokenManagerType(tokenManagerType);
     }
 
@@ -112,6 +119,7 @@ contract TokenHandler is ITokenHandler, ITokenManagerType, ReentrancyGuard {
         if (diff < amount) {
             amount = diff;
         }
+
         return amount;
     }
 
@@ -130,6 +138,7 @@ contract TokenHandler is ITokenHandler, ITokenManagerType, ReentrancyGuard {
         if (diff < amount) {
             amount = diff;
         }
+
         return amount;
     }
 

contracts/interchain-token/InterchainToken.sol

@@ -22,7 +22,7 @@ contract InterchainToken is BaseInterchainToken, ERC20Permit, Distributable, IIn
     string public symbol;
     uint8 public decimals;
     bytes32 internal tokenId;
-    address internal immutable interchainTokenServiceAddress;
+    address internal immutable interchainTokenService_;
 
     // bytes32(uint256(keccak256('interchain-token-initialized')) - 1);
     bytes32 internal constant INITIALIZED_SLOT = 0xc778385ecb3e8cecb82223fa1f343ec6865b2d64c65b0c15c7e8aef225d9e214;
@@ -31,11 +31,12 @@ contract InterchainToken is BaseInterchainToken, ERC20Permit, Distributable, IIn
      * @notice Constructs the InterchainToken contract.
      * @dev Makes the implementation act as if it has been setup already to disallow calls to init() (even though that would not achieve anything really).
      */
-    constructor(address interchainTokenServiceAddress_) {
+    constructor(address interchainTokenServiceAddress) {
         _initialize();
 
-        if (interchainTokenServiceAddress_ == address(0)) revert InterchainTokenServiceAddressZero();
-        interchainTokenServiceAddress = interchainTokenServiceAddress_;
+        if (interchainTokenServiceAddress == address(0)) revert InterchainTokenServiceAddressZero();
+
+        interchainTokenService_ = interchainTokenServiceAddress;
     }
 
     /**
@@ -62,7 +63,7 @@ contract InterchainToken is BaseInterchainToken, ERC20Permit, Distributable, IIn
      * @return address The interchain token service contract
      */
     function interchainTokenService() public view override(BaseInterchainToken, IInterchainToken) returns (address) {
-        return interchainTokenServiceAddress;
+        return interchainTokenService_;
     }
 
     /**
@@ -100,7 +101,12 @@ contract InterchainToken is BaseInterchainToken, ERC20Permit, Distributable, IIn
         decimals = tokenDecimals;
         tokenId = tokenId_;
 
-        _addDistributor(interchainTokenServiceAddress);
+        /**
+         * @dev Set the token service as a distributor to allow it to mint and burn tokens.
+         * Also add the provided address as a distributor. If `address(0)` was provided,
+         * add it as a distributor to allow anyone to easily check that no custom distributor was set.
+         */
+        _addDistributor(interchainTokenService_);
         _addDistributor(distributor);
 
         _setNameHash(tokenName);

contracts/token-manager/TokenManager.sol

@@ -171,6 +171,11 @@ contract TokenManager is ITokenManager, Operatable, FlowLimit, Implementation {
      * @notice A function to renew approval to the service if we need to.
      */
     function approveService() external onlyService {
+        /**
+         * @dev Some tokens may not obey the infinite approval.
+         * Even so, it is unexpected to run out of allowance in practice.
+         * If needed, we can upgrade to allow replenishing the allowance in the future.
+         */
         IERC20(this.tokenAddress()).safeCall(abi.encodeWithSelector(IERC20.approve.selector, interchainTokenService, UINT256_MAX));
     }
 

slither.config.json

@@ -1,4 +1,4 @@
 {
-    "detectors_to_exclude": "assembly,low-level-calls,solc-version,timestamp,dead-code",
+    "detectors_to_exclude": "assembly,low-level-calls,solc-version,timestamp,dead-code,controlled-delegatecall",
     "filter_paths": "(contracts/test/|node_modules/)"
 }

test/TokenManager.js

@@ -66,22 +66,34 @@ describe('Token Manager', () => {
         );
     });
 
+    it('Should revert on addFlowIn when calling directly', async () => {
+        await expectRevert((gasOptions) => TestTokenManager.addFlowIn(0, gasOptions), TestTokenManager, 'NotService', [owner.address]);
+    });
+
+    it('Should revert on addFlowOut when calling directly', async () => {
+        await expectRevert((gasOptions) => TestTokenManager.addFlowOut(0, gasOptions), TestTokenManager, 'NotService', [owner.address]);
+    });
+
+    it('Should revert on approveService when calling directly', async () => {
+        await expectRevert((gasOptions) => TestTokenManager.approveService(gasOptions), TestTokenManager, 'NotService', [owner.address]);
+    });
+
     it('Should return the correct parameters for a token manager', async () => {
         const expectedParams = defaultAbiCoder.encode(['bytes', 'address'], [toUtf8Bytes(owner.address), token.address]);
         const params = await TestTokenManager.params(toUtf8Bytes(owner.address), token.address);
         expect(expectedParams).to.eq(params);
     });
 
-    describe.skip('Bytecode checks [ @skip-on-coverage ]', () => {
+    describe('Bytecode checks [ @skip-on-coverage ]', () => {
         it('Should preserve the same proxy bytecode for each EVM', async () => {
             const proxyFactory = await ethers.getContractFactory('TokenManagerProxy', owner);
             const proxyBytecode = proxyFactory.bytecode;
             const proxyBytecodeHash = keccak256(proxyBytecode);
 
             const expected = {
-                istanbul: '0xce3ee5c04c84351d193a6e5dc52e34702039a6083437b077367bac26da57103c',
-                berlin: '0xea7ab1f8727ce63dd60f1b7c6770723259b7ac2ce69a74046509e2a65cd4b899',
-                london: '0x97da1989bb59bf727d23961f163900ce0dcab3dafa2b3fa0aec39f09c5bd233e',
+                istanbul: '0x8e833c9efc444489f636e5582be33991c73273049065a071e414b3fc6e64bb54',
+                berlin: '0x6e1fe42f41fb15015c4fc5b702747255296568a52b019b8f061c000921433aa3',
+                london: '0x6ff5905df915a3661814814739766a16d87576480c270fda42c04c25955716ad',
             }[getEVMVersion()];
 
             expect(proxyBytecodeHash).to.be.equal(expected);

test/TokenService.js

@@ -400,29 +400,52 @@ describe('Interchain Token Service', () => {
                 service,
                 'ZeroAddress',
             );
+        });
 
-            it('Should revert on invalid token manager', async () => {
-                await expectRevert(
-                    (gasOptions) =>
-                        deployInterchainTokenService(
-                            wallet,
-                            create3Deployer.address,
-                            tokenManagerDeployer.address,
-                            interchainTokenDeployer.address,
-                            gateway.address,
-                            gasService.address,
-                            interchainTokenFactoryAddress,
-                            tokenManager.address,
-                            AddressZero,
-                            chainName,
-                            [],
-                            deploymentKey,
-                            gasOptions,
-                        ),
-                    service,
-                    'ZeroAddress',
-                );
-            });
+        it('Should revert on invalid token handler', async () => {
+            await expectRevert(
+                (gasOptions) =>
+                    deployInterchainTokenService(
+                        wallet,
+                        create3Deployer.address,
+                        tokenManagerDeployer.address,
+                        interchainTokenDeployer.address,
+                        gateway.address,
+                        gasService.address,
+                        interchainTokenFactoryAddress,
+                        tokenManager.address,
+                        AddressZero,
+                        chainName,
+                        [],
+                        deploymentKey,
+                        gasOptions,
+                    ),
+                service,
+                'ZeroAddress',
+            );
+        });
+
+        it('Should revert on invalid token manager', async () => {
+            await expectRevert(
+                (gasOptions) =>
+                    deployInterchainTokenService(
+                        wallet,
+                        create3Deployer.address,
+                        tokenManagerDeployer.address,
+                        interchainTokenDeployer.address,
+                        gateway.address,
+                        gasService.address,
+                        interchainTokenFactoryAddress,
+                        tokenManager.address,
+                        AddressZero,
+                        chainName,
+                        [],
+                        deploymentKey,
+                        gasOptions,
+                    ),
+                service,
+                'ZeroAddress',
+            );
         });
 
         it('Should return the token manager implementation', async () => {
@@ -790,6 +813,7 @@ describe('Interchain Token Service', () => {
 
             await expectRevert((gasOptions) => implementation.tokenAddress(gasOptions), implementation, 'NotSupported');
             await expectRevert((gasOptions) => implementation.interchainTokenId(gasOptions), implementation, 'NotSupported');
+            await expectRevert((gasOptions) => implementation.implementationType(gasOptions), implementation, 'NotSupported');
         });
 
         it('Should deploy a lock/unlock token manager', async () => {