From e23ef80f9b6d362308ddc1ce903244fd8427f93a Mon Sep 17 00:00:00 2001 From: ahramy Date: Mon, 4 Nov 2024 12:04:19 -0800 Subject: [PATCH 1/7] feat(its): prevent invalid flow limit --- contracts/interfaces/IFlowLimit.sol | 1 + contracts/utils/FlowLimit.sol | 2 ++ test/InterchainTokenService.js | 9 +++++++++ 3 files changed, 12 insertions(+) diff --git a/contracts/interfaces/IFlowLimit.sol b/contracts/interfaces/IFlowLimit.sol index 978c8176..796b4597 100644 --- a/contracts/interfaces/IFlowLimit.sol +++ b/contracts/interfaces/IFlowLimit.sol @@ -8,6 +8,7 @@ pragma solidity ^0.8.0; */ interface IFlowLimit { error FlowLimitExceeded(uint256 limit, uint256 flowAmount, address tokenManager); + error InvalidFlowLimit(uint256 flowLimit, bytes32 tokenId); event FlowLimitSet(bytes32 indexed tokenId, address operator, uint256 flowLimit_); diff --git a/contracts/utils/FlowLimit.sol b/contracts/utils/FlowLimit.sol index 9598a925..296cf3bc 100644 --- a/contracts/utils/FlowLimit.sol +++ b/contracts/utils/FlowLimit.sol @@ -33,6 +33,8 @@ contract FlowLimit is IFlowLimit { * @param tokenId The id of the token to set the flow limit for. */ function _setFlowLimit(uint256 flowLimit_, bytes32 tokenId) internal { + if (flowLimit_ == type(uint256).max) revert InvalidFlowLimit(flowLimit_, tokenId); + assembly { sstore(FLOW_LIMIT_SLOT, flowLimit_) } diff --git a/test/InterchainTokenService.js b/test/InterchainTokenService.js index f12ecc7e..b9d66099 100644 --- a/test/InterchainTokenService.js +++ b/test/InterchainTokenService.js @@ -2721,6 +2721,15 @@ describe('Interchain Token Service', () => { await tokenManager.setFlowLimit(flowLimit).then((tx) => tx.wait); }); + it('Should be reverted setting invalid flow limit', async () => { + const invalidFlowLimit = MaxUint256; + + await expectRevert((gasOptions) => tokenManager.setFlowLimit(invalidFlowLimit, gasOptions), tokenManager, 'InvalidFlowLimit', [ + invalidFlowLimit, + tokenId, + ]); + }); + it('Should be able to send token only if it does not trigger the mint limit', async () => { await service.interchainTransfer(tokenId, destinationChain, destinationAddress, sendAmount, '0x', 0).then((tx) => tx.wait); From b875a83cb6d02bd25075b6fd4571445cc1c5f059 Mon Sep 17 00:00:00 2001 From: ahramy Date: Mon, 4 Nov 2024 12:11:37 -0800 Subject: [PATCH 2/7] Update FlowLimit.sol --- contracts/utils/FlowLimit.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contracts/utils/FlowLimit.sol b/contracts/utils/FlowLimit.sol index 296cf3bc..339a04b1 100644 --- a/contracts/utils/FlowLimit.sol +++ b/contracts/utils/FlowLimit.sol @@ -29,7 +29,9 @@ contract FlowLimit is IFlowLimit { /** * @notice Internal function to set the flow limit. - * @param flowLimit_ The value to set the flow limit to. + * @param flowLimit_ The value to set the flow limit to. Must not be set to `uint256.max`. + * Only the operator can set the flow limit for their own token; however, an incorrect setting + * may result in a denial of service due to potential arithmetic underflow or overflow. * @param tokenId The id of the token to set the flow limit for. */ function _setFlowLimit(uint256 flowLimit_, bytes32 tokenId) internal { From 2d3293975a83c344243eb35659625437a0afdd35 Mon Sep 17 00:00:00 2001 From: ahramy Date: Tue, 5 Nov 2024 12:28:37 -0800 Subject: [PATCH 3/7] updated flow and unit test --- contracts/interfaces/IFlowLimit.sol | 3 +- contracts/utils/FlowLimit.sol | 6 ++- test/InterchainTokenService.js | 77 ++++++++++++++++++++++++----- 3 files changed, 71 insertions(+), 15 deletions(-) diff --git a/contracts/interfaces/IFlowLimit.sol b/contracts/interfaces/IFlowLimit.sol index 796b4597..188e7495 100644 --- a/contracts/interfaces/IFlowLimit.sol +++ b/contracts/interfaces/IFlowLimit.sol @@ -8,7 +8,8 @@ pragma solidity ^0.8.0; */ interface IFlowLimit { error FlowLimitExceeded(uint256 limit, uint256 flowAmount, address tokenManager); - error InvalidFlowLimit(uint256 flowLimit, bytes32 tokenId); + error FlowAdditionOverflow(uint256 flowToAdd, uint256 flowAmount, address tokenManager); + error FlowLimitOverflow(uint256 flowLimit, uint256 flowToCompare, address tokenManager); event FlowLimitSet(bytes32 indexed tokenId, address operator, uint256 flowLimit_); diff --git a/contracts/utils/FlowLimit.sol b/contracts/utils/FlowLimit.sol index 339a04b1..4dc31584 100644 --- a/contracts/utils/FlowLimit.sol +++ b/contracts/utils/FlowLimit.sol @@ -35,8 +35,6 @@ contract FlowLimit is IFlowLimit { * @param tokenId The id of the token to set the flow limit for. */ function _setFlowLimit(uint256 flowLimit_, bytes32 tokenId) internal { - if (flowLimit_ == type(uint256).max) revert InvalidFlowLimit(flowLimit_, tokenId); - assembly { sstore(FLOW_LIMIT_SLOT, flowLimit_) } @@ -104,6 +102,10 @@ contract FlowLimit is IFlowLimit { flowToCompare := sload(slotToCompare) } + // Overflow checks for flowToAdd + flowAmount and flowToCompare + flowLimit_ + if (flowToAdd > type(uint256).max - flowAmount) revert FlowAdditionOverflow(flowAmount, flowToAdd, address(this)); + if (flowToCompare > type(uint256).max - flowLimit_) revert FlowLimitOverflow(flowLimit_, flowToCompare, address(this)); + if (flowToAdd + flowAmount > flowToCompare + flowLimit_) revert FlowLimitExceeded((flowToCompare + flowLimit_), flowToAdd + flowAmount, address(this)); if (flowAmount > flowLimit_) revert FlowLimitExceeded(flowLimit_, flowAmount, address(this)); diff --git a/test/InterchainTokenService.js b/test/InterchainTokenService.js index b9d66099..0f427258 100644 --- a/test/InterchainTokenService.js +++ b/test/InterchainTokenService.js @@ -2714,22 +2714,13 @@ describe('Interchain Token Service', () => { let tokenManager, tokenId; const sendAmount = 1234; const flowLimit = (sendAmount * 3) / 2; - const mintAmount = flowLimit * 3; + const mintAmount = MaxUint256; before(async () => { [, tokenManager, tokenId] = await deployFunctions.mintBurn(service, 'Test Token Lock Unlock', 'TT', 12, mintAmount); await tokenManager.setFlowLimit(flowLimit).then((tx) => tx.wait); }); - it('Should be reverted setting invalid flow limit', async () => { - const invalidFlowLimit = MaxUint256; - - await expectRevert((gasOptions) => tokenManager.setFlowLimit(invalidFlowLimit, gasOptions), tokenManager, 'InvalidFlowLimit', [ - invalidFlowLimit, - tokenId, - ]); - }); - it('Should be able to send token only if it does not trigger the mint limit', async () => { await service.interchainTransfer(tokenId, destinationChain, destinationAddress, sendAmount, '0x', 0).then((tx) => tx.wait); @@ -2755,10 +2746,10 @@ describe('Interchain Token Service', () => { expect(flowIn).to.eq(0); expect(flowOut).to.eq(sendAmount); - async function receiveToken(sendAmount) { + async function receiveToken(amount) { const payload = defaultAbiCoder.encode( ['uint256', 'bytes32', 'bytes', 'bytes', 'uint256', 'bytes'], - [MESSAGE_TYPE_INTERCHAIN_TRANSFER, tokenId, hexlify(wallet.address), wallet.address, sendAmount, '0x'], + [MESSAGE_TYPE_INTERCHAIN_TRANSFER, tokenId, hexlify(wallet.address), wallet.address, amount, '0x'], ); const commandId = await approveContractCall(gateway, destinationChain, service.address, service.address, payload); @@ -2785,6 +2776,68 @@ describe('Interchain Token Service', () => { ]); }); + it('Should revert if the flow limit overflows', async () => { + const tokenFlowLimit = await service.flowLimit(tokenId); + expect(tokenFlowLimit).to.eq(flowLimit); + + let flowIn = await service.flowInAmount(tokenId); + let flowOut = await service.flowOutAmount(tokenId); + + expect(flowIn).to.eq(sendAmount); + expect(flowOut).to.eq(sendAmount); + + let newFlowLimit = MaxUint256; + let newSendAmount = 1; + + await tokenManager.setFlowLimit(newFlowLimit).then((tx) => tx.wait); + + async function receiveToken(amount) { + const payload = defaultAbiCoder.encode( + ['uint256', 'bytes32', 'bytes', 'bytes', 'uint256', 'bytes'], + [MESSAGE_TYPE_INTERCHAIN_TRANSFER, tokenId, hexlify(wallet.address), wallet.address, amount, '0x'], + ); + const commandId = await approveContractCall(gateway, destinationChain, service.address, service.address, payload); + + return service.execute(commandId, destinationChain, service.address, payload); + } + + const errorSignatureHash = id('FlowLimitOverflow(uint256,uint256,address)'); + const selector = errorSignatureHash.substring(0, 10); + const errorData = defaultAbiCoder.encode(['uint256', 'uint256', 'address'], [newFlowLimit, flowIn, tokenManager.address]); + + await expectRevert((gasOptions) => receiveToken(newSendAmount, gasOptions), service, 'GiveTokenFailed', [ + selector + errorData.substring(2), + ]); + }); + + it('Should revert if the flow addition overflows', async () => { + let newFlowLimit = MaxUint256; + let newSendAmount = MaxUint256; + + const tokenFlowLimit = await service.flowLimit(tokenId); + expect(tokenFlowLimit).to.eq(MaxUint256); + + let flowIn = await service.flowInAmount(tokenId); + let flowOut = await service.flowOutAmount(tokenId); + + expect(flowIn).to.eq(sendAmount); + expect(flowOut).to.eq(sendAmount); + + await tokenManager.setFlowLimit(newFlowLimit).then((tx) => tx.wait); + + const errorSignatureHash = id('FlowAdditionOverflow(uint256,uint256,address)'); + const selector = errorSignatureHash.substring(0, 10); + const errorData = defaultAbiCoder.encode(['uint256', 'uint256', 'address'], [newSendAmount, flowOut, tokenManager.address]); + + await expectRevert( + (gasOptions) => + service.interchainTransfer(tokenId, destinationChain, destinationAddress, newSendAmount, '0x', 0, gasOptions), + service, + 'TakeTokenFailed', + [selector + errorData.substring(2)], + ); + }); + it('Should be able to set flow limits for each token manager', async () => { const tokenIds = []; const tokenManagers = []; From f150ff15b7d8cc779c4e53322ca66b9ce1da750e Mon Sep 17 00:00:00 2001 From: ahramy Date: Tue, 5 Nov 2024 12:33:06 -0800 Subject: [PATCH 4/7] Update InterchainTokenService.js --- test/InterchainTokenService.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/InterchainTokenService.js b/test/InterchainTokenService.js index 0f427258..da89bf76 100644 --- a/test/InterchainTokenService.js +++ b/test/InterchainTokenService.js @@ -2780,14 +2780,14 @@ describe('Interchain Token Service', () => { const tokenFlowLimit = await service.flowLimit(tokenId); expect(tokenFlowLimit).to.eq(flowLimit); - let flowIn = await service.flowInAmount(tokenId); - let flowOut = await service.flowOutAmount(tokenId); + const flowIn = await service.flowInAmount(tokenId); + const flowOut = await service.flowOutAmount(tokenId); expect(flowIn).to.eq(sendAmount); expect(flowOut).to.eq(sendAmount); - let newFlowLimit = MaxUint256; - let newSendAmount = 1; + const newFlowLimit = MaxUint256; + const newSendAmount = 1; await tokenManager.setFlowLimit(newFlowLimit).then((tx) => tx.wait); @@ -2811,18 +2811,18 @@ describe('Interchain Token Service', () => { }); it('Should revert if the flow addition overflows', async () => { - let newFlowLimit = MaxUint256; - let newSendAmount = MaxUint256; - const tokenFlowLimit = await service.flowLimit(tokenId); expect(tokenFlowLimit).to.eq(MaxUint256); - let flowIn = await service.flowInAmount(tokenId); - let flowOut = await service.flowOutAmount(tokenId); + const flowIn = await service.flowInAmount(tokenId); + const flowOut = await service.flowOutAmount(tokenId); expect(flowIn).to.eq(sendAmount); expect(flowOut).to.eq(sendAmount); + const newFlowLimit = MaxUint256; + const newSendAmount = MaxUint256; + await tokenManager.setFlowLimit(newFlowLimit).then((tx) => tx.wait); const errorSignatureHash = id('FlowAdditionOverflow(uint256,uint256,address)'); From 4d6e8042b82e5ee92473c258507c25b921b39c86 Mon Sep 17 00:00:00 2001 From: ahramy Date: Tue, 5 Nov 2024 14:33:33 -0800 Subject: [PATCH 5/7] Update FlowLimit.sol --- contracts/utils/FlowLimit.sol | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/contracts/utils/FlowLimit.sol b/contracts/utils/FlowLimit.sol index 4dc31584..9c439bbf 100644 --- a/contracts/utils/FlowLimit.sol +++ b/contracts/utils/FlowLimit.sol @@ -29,9 +29,7 @@ contract FlowLimit is IFlowLimit { /** * @notice Internal function to set the flow limit. - * @param flowLimit_ The value to set the flow limit to. Must not be set to `uint256.max`. - * Only the operator can set the flow limit for their own token; however, an incorrect setting - * may result in a denial of service due to potential arithmetic underflow or overflow. + * @param flowLimit_ The value to set the flow limit to. * @param tokenId The id of the token to set the flow limit for. */ function _setFlowLimit(uint256 flowLimit_, bytes32 tokenId) internal { From e53d2691089e9049906ee40f84164749c874a6e2 Mon Sep 17 00:00:00 2001 From: ahramy Date: Tue, 5 Nov 2024 14:35:45 -0800 Subject: [PATCH 6/7] Update InterchainTokenService.js --- test/InterchainTokenService.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/test/InterchainTokenService.js b/test/InterchainTokenService.js index da89bf76..e9568011 100644 --- a/test/InterchainTokenService.js +++ b/test/InterchainTokenService.js @@ -2786,8 +2786,8 @@ describe('Interchain Token Service', () => { expect(flowIn).to.eq(sendAmount); expect(flowOut).to.eq(sendAmount); - const newFlowLimit = MaxUint256; const newSendAmount = 1; + const newFlowLimit = MaxUint256; await tokenManager.setFlowLimit(newFlowLimit).then((tx) => tx.wait); @@ -2820,11 +2820,8 @@ describe('Interchain Token Service', () => { expect(flowIn).to.eq(sendAmount); expect(flowOut).to.eq(sendAmount); - const newFlowLimit = MaxUint256; const newSendAmount = MaxUint256; - await tokenManager.setFlowLimit(newFlowLimit).then((tx) => tx.wait); - const errorSignatureHash = id('FlowAdditionOverflow(uint256,uint256,address)'); const selector = errorSignatureHash.substring(0, 10); const errorData = defaultAbiCoder.encode(['uint256', 'uint256', 'address'], [newSendAmount, flowOut, tokenManager.address]); From 1ae07e538ef4555b21c2240fabf947ff6b2b88e0 Mon Sep 17 00:00:00 2001 From: ahramy Date: Tue, 5 Nov 2024 14:39:03 -0800 Subject: [PATCH 7/7] Update IFlowLimit.sol --- contracts/interfaces/IFlowLimit.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/interfaces/IFlowLimit.sol b/contracts/interfaces/IFlowLimit.sol index 188e7495..c4c83334 100644 --- a/contracts/interfaces/IFlowLimit.sol +++ b/contracts/interfaces/IFlowLimit.sol @@ -8,7 +8,7 @@ pragma solidity ^0.8.0; */ interface IFlowLimit { error FlowLimitExceeded(uint256 limit, uint256 flowAmount, address tokenManager); - error FlowAdditionOverflow(uint256 flowToAdd, uint256 flowAmount, address tokenManager); + error FlowAdditionOverflow(uint256 flowAmount, uint256 flowToAdd, address tokenManager); error FlowLimitOverflow(uint256 flowLimit, uint256 flowToCompare, address tokenManager); event FlowLimitSet(bytes32 indexed tokenId, address operator, uint256 flowLimit_);