TransactionManager - Missing nonReentrant modifier on removeLiquidity
Resolution\\nThis issue has been fixed.\\nThe `removeLiquidity` function does not have a `nonReentrant` modifier.\\n```\\n/\\*\\*\\n \\* @notice This is used by any router to decrease their available\\n \\* liquidity for a given asset.\\n \\* @param shares The amount of liquidity to remove for the router in shares\\n \\* @param assetId The address (or `address(0)` if native asset) of the\\n \\* asset you're removing liquidity for\\n \\* @param recipient The address that will receive the liquidity being removed\\n \\*/\\nfunction removeLiquidity(\\n  uint256 shares,\\n  address assetId,\\n  address payable recipient\\n) external override {\\n  // Sanity check: recipient is sensible\\n  require(recipient != address(0), "#RL:007");\\n\\n  // Sanity check: nonzero shares\\n  require(shares > 0, "#RL:035");\\n\\n  // Get stored router shares\\n  uint256 routerShares = issuedShares[msg.sender][assetId];\\n\\n  // Get stored outstanding shares\\n  uint256 outstanding = outstandingShares[assetId];\\n\\n  // Sanity check: owns enough shares\\n  require(routerShares >= shares, "#RL:018");\\n\\n  // Convert shares to amount\\n  uint256 amount = getAmountFromIssuedShares(\\n    shares,\\n    outstanding,\\n    Asset.getOwnBalance(assetId)\\n  );\\n\\n  // Update router issued shares\\n  // NOTE: unchecked due to require above\\n  unchecked {\\n    issuedShares[msg.sender][assetId] = routerShares - shares;\\n  }\\n\\n  // Update the total shares for asset\\n  outstandingShares[assetId] = outstanding - shares;\\n\\n  // Transfer from contract to specified recipient\\n  Asset.transferAsset(assetId, recipient, amount);\\n\\n  // Emit event\\n  emit LiquidityRemoved(\\n    msg.sender,\\n    assetId,\\n    shares,\\n    amount,\\n    recipient\\n  );\\n}\\n```\\n\\nAssuming we're dealing with a token contract that allows execution of third-party-supplied code, that means it is possible to leave the `TransactionManager` contract in one of the functions that call into the token contract and then reenter via `removeLiquidity`. Alternatively, we can leave the contract in `removeLiquidity` and reenter through an arbitrary external function, even if it has a `nonReentrant` modifier.\\nExample\\nAssume a token contract allows the execution of third-party-supplied code in its `transfer` function before the actual balance change takes place. If a router calls `removeLiquidity` with half of their shares and then, in a reentering `removeLiquidity` call, supplies the other half of their shares, they will receive more tokens than if they had liquidated all their shares at once because the reentering call occurs after the (first half of the) shares have been burnt but before the corresponding amount of tokens has actually been transferred out of the contract, leading to an artificially increased share value in the reentering call. Similarly, reentering the contract with a `fulfill` call on the receiving chain instead of a second `removeLiquidity` would `transfer` too many tokens to the recipient due to the artificially inflated share value.
While tokens that behave as described in the example might be rare or not exist at all, caution is advised when integrating with unknown tokens or calling untrusted code in general. We strongly recommend adding a `nonReentrant` modifier to `removeLiquidity`.
```\\n/\\*\\*\\n \\* @notice This is used by any router to decrease their available\\n \\* liquidity for a given asset.\\n \\* @param shares The amount of liquidity to remove for the router in shares\\n \\* @param assetId The address (or `address(0)` if native asset) of the\\n \\* asset you're removing liquidity for\\n \\* @param recipient The address that will receive the liquidity being removed\\n \\*/\\nfunction removeLiquidity(\\n  uint256 shares,\\n  address assetId,\\n  address payable recipient\\n) external override {\\n  // Sanity check: recipient is sensible\\n  require(recipient != address(0), "#RL:007");\\n\\n  // Sanity check: nonzero shares\\n  require(shares > 0, "#RL:035");\\n\\n  // Get stored router shares\\n  uint256 routerShares = issuedShares[msg.sender][assetId];\\n\\n  // Get stored outstanding shares\\n  uint256 outstanding = outstandingShares[assetId];\\n\\n  // Sanity check: owns enough shares\\n  require(routerShares >= shares, "#RL:018");\\n\\n  // Convert shares to amount\\n  uint256 amount = getAmountFromIssuedShares(\\n    shares,\\n    outstanding,\\n    Asset.getOwnBalance(assetId)\\n  );\\n\\n  // Update router issued shares\\n  // NOTE: unchecked due to require above\\n  unchecked {\\n    issuedShares[msg.sender][assetId] = routerShares - shares;\\n  }\\n\\n  // Update the total shares for asset\\n  outstandingShares[assetId] = outstanding - shares;\\n\\n  // Transfer from contract to specified recipient\\n  Asset.transferAsset(assetId, recipient, amount);\\n\\n  // Emit event\\n  emit LiquidityRemoved(\\n    msg.sender,\\n    assetId,\\n    shares,\\n    amount,\\n    recipient\\n  );\\n}\\n```\\n