diff --git a/contracts/dex-core/Dex223Factory.sol b/contracts/dex-core/Dex223Factory.sol index 80f8240..ef9b111 100644 --- a/contracts/dex-core/Dex223Factory.sol +++ b/contracts/dex-core/Dex223Factory.sol @@ -13,7 +13,7 @@ import './Dex223Pool.sol'; /// @title Canonical Uniswap V3 factory /// @notice Deploys Uniswap V3 pools and manages ownership and control over pool protocol fees -contract Dex223Factory is IDex223Factory, UniswapV3PoolDeployer, NoDelegateCall { +contract Dex223Factory is IDex223Factory, Dex223PoolDeployer, NoDelegateCall { // @inheritdoc IUniswapV3Factory address public override owner; diff --git a/contracts/dex-core/Dex223Pool.sol b/contracts/dex-core/Dex223Pool.sol index db4c786..bbb45c1 100644 --- a/contracts/dex-core/Dex223Pool.sol +++ b/contracts/dex-core/Dex223Pool.sol @@ -164,28 +164,34 @@ contract Dex223Pool is IUniswapV3Pool, NoDelegateCall, PeripheryValidation { maxLiquidityPerTick = Tick.tickSpacingToMaxLiquidityPerTick(_tickSpacing); } + // @audit-fix V7: Added zero-address validation for all parameters. + // If pool_lib or quote_lib is set to address(0), all delegatecall-based functions + // (swap, mint, burn, collect) will silently succeed but return empty data, + // causing abi.decode to revert with an opaque error. Worse, if converter is + // address(0), optimisticDelivery in the pool library would call convert on + // address(0), silently failing and potentially locking user funds. + // Also added a guard to prevent re-initialization after pool_lib is already set, + // which would allow the factory to change the pool's logic contract mid-flight. function set( - //address _t0erc20, - //address _t1erc20, address _t0erc223, address _t1erc223, - //uint24 _fee, - //int24 _tickSpacing, address _library, address _quote_library, address _converter ) external { - require(msg.sender == factory); + require(msg.sender == factory, "POOL: NOT_FACTORY"); + require(pool_lib == address(0), "POOL: ALREADY_SET"); + require(_t0erc223 != address(0), "POOL: ZERO_T0_ERC223"); + require(_t1erc223 != address(0), "POOL: ZERO_T1_ERC223"); + require(_library != address(0), "POOL: ZERO_LIBRARY"); + require(_quote_library != address(0), "POOL: ZERO_QUOTE_LIB"); + require(_converter != address(0), "POOL: ZERO_CONVERTER"); pool_lib = _library; quote_lib = _quote_library; - //token0.erc20 = _t0erc20; - //token1.erc20 = _t1erc20; token0.erc223 = _t0erc223; token1.erc223 = _t1erc223; converter = ITokenStandardConverter(_converter); - //fee = _fee; - //maxLiquidityPerTick = Tick.tickSpacingToMaxLiquidityPerTick(_tickSpacing); } /** @@ -197,12 +203,37 @@ contract Dex223Pool is IUniswapV3Pool, NoDelegateCall, PeripheryValidation { */ function tokenReceived(address _from, uint _value, bytes memory _data) public returns (bytes4) { + // @audit-fix V1: Validate that the calling token is one of the pool's tokens. + // Without this check, any ERC-223 contract can call tokenReceived and + // set swap_sender to an arbitrary _from address, potentially spoofing + // deposit credits for tokens unrelated to this pool. + require( + msg.sender == token0.erc223 || msg.sender == token1.erc223, + "POOL: INVALID_TOKEN" + ); + require(!erc223ReentrancyLock); // Specific reentrancy protection for ERC-223 deposits erc223ReentrancyLock = true; swap_sender = _from; erc223deposit[_from][msg.sender] += _value; // add token to user balance if (_data.length != 0) { + // @audit-fix V1: Restrict delegatecall to only allowed pool functions. + // The original code passed arbitrary _data to delegatecall, enabling an attacker + // to call any function (including set(), setFeeProtocol(), or selfdestruct gadgets) + // in the context of the pool contract. We whitelist only swap() and swapExactInput() + // selectors, which are the legitimate operations during an ERC-223 token deposit. + require(_data.length >= 4, "POOL: DATA_TOO_SHORT"); + bytes4 selector; + assembly { + selector := mload(add(_data, 32)) + } + require( + selector == bytes4(keccak256("swap(address,bool,int256,uint160,bool,bytes)")) || + selector == bytes4(keccak256("swapExactInput(address,bool,int256,uint256,uint160,bool,bytes,uint256,bool)")), + "POOL: FORBIDDEN_SELECTOR" + ); + (bool success, bytes memory _data_) = address(this).delegatecall(_data); delete(_data); @@ -213,7 +244,7 @@ contract Dex223Pool is IUniswapV3Pool, NoDelegateCall, PeripheryValidation { if (erc223deposit[_from][msg.sender] != 0) { TransferHelper.safeTransfer(msg.sender, _from, erc223deposit[_from][msg.sender]); erc223deposit[_from][msg.sender] = 0; - } + } erc223ReentrancyLock = false; swap_sender = address(0); @@ -453,15 +484,26 @@ contract Dex223Pool is IUniswapV3Pool, NoDelegateCall, PeripheryValidation { if (success) { (amount0, amount1) = abi.decode(retdata, (int256, int256)); } else { - string memory val = abi.decode(retdata, (string)); + // @audit-fix V2: Properly propagate revert data from the delegatecall target. + // The original code attempted `abi.decode(retdata, (string))` which fails + // when the revert reason is not ABI-encoded as a string (e.g. custom errors, + // Panic(uint256), or raw revert bytes). This would cause a secondary decode + // revert that masks the original error message, making debugging impossible. + // The fix: relay the raw revert data directly, preserving the original error. assembly { - let ptr := mload(0x40) - mstore(ptr, val) - revert(ptr, 32) + revert(add(retdata, 32), mload(retdata)) } } } + // @audit-fix V4: quoteSwap was not protected by the lock modifier, meaning it could + // be called reentrantly or concurrently with swap operations. While the quote_lib + // ends with a revert (so state changes are rolled back within the delegatecall), + // the function itself was still externally callable during a locked state. + // Added `lock` to prevent reentrancy and ensure consistent state reads. + // Also added `view`-like documentation: this function is intended for off-chain + // simulation only (via eth_call). On-chain execution will always revert because + // quote_lib's quoteSwap reverts intentionally to return data. function quoteSwap( address recipient, bool zeroForOne, @@ -469,35 +511,54 @@ contract Dex223Pool is IUniswapV3Pool, NoDelegateCall, PeripheryValidation { uint160 sqrtPriceLimitX96, bool prefer223, bytes memory data - ) external returns (int256 delta) { + ) external lock returns (int256 delta) { // quoteSwap performs a `revert()` once it will reach IUniswapV3SwapCallback invocation // and passes callback values back to retdata. (bool success, bytes memory retdata) = quote_lib.delegatecall(abi.encodeWithSignature("quoteSwap(address,bool,int256,uint160,bool,bytes)", recipient, zeroForOne, amountSpecified, sqrtPriceLimitX96, prefer223, data)); + require(retdata.length >= 32, "POOL: QUOTE_FAILED"); (int256 _d0) = abi.decode(retdata, (int256)); - emit Delta0(_d0); // This event is preserved for debugging purposes, - // this function is never supposed to be called in practice - // since the caller must simulate the transaction - //and get the return value instead. + emit Delta0(_d0); return _d0; } + // @audit-fix V5: Unrestricted receive() allows anyone to send ETH to the pool, + // permanently locking those funds since the pool has no general-purpose ETH + // withdrawal mechanism (withdrawEther is owner-only and unrelated to user deposits). + // The only legitimate ETH sender is a WETH contract during unwrapWETH9(). + // We restrict to token0.erc20 or token1.erc20 which covers the case where + // one of the pool tokens is WETH and it sends ETH during withdraw(). receive() external payable { -// require(msg.sender == WETH9, 'Not WETH9'); + require( + msg.sender == token0.erc20 || msg.sender == token1.erc20, + "POOL: ETH_REJECTED" + ); } - function unwrapWETH9(address recipient, address WETH9, uint256 amountOut) private { + // @audit-fix V8: Multiple issues fixed in unwrapWETH9: + // 1. `require(msg.sender != address(this))` - This is always true for a private function + // called from an external function. It was likely meant to prevent self-calls via + // delegatecall, but it doesn't serve that purpose. Removed dead code. + // 2. Added recipient != address(0) check to prevent burning ETH. + // 3. Added amountOut > 0 check as a precondition instead of wrapping in + // `if (balanceWETH9 > 0)` which would silently skip a zero unwrap. + function unwrapWETH9(address recipient, address WETH9, uint256 amountOut) private { + require(recipient != address(0), "POOL: ZERO_RECIPIENT"); + require(amountOut > 0, "POOL: ZERO_UNWRAP"); uint256 balanceWETH9 = IWETH9(WETH9).balanceOf(address(this)); require(balanceWETH9 >= amountOut, 'Insufficient WETH9'); - require(msg.sender != address(this)); - if (balanceWETH9 > 0) { - IWETH9(WETH9).withdraw(amountOut); - TransferHelper.safeTransferETH(recipient, amountOut); - } + IWETH9(WETH9).withdraw(amountOut); + TransferHelper.safeTransferETH(recipient, amountOut); } /// @dev to make direct swap via pool with deadline and slippage + // @audit-fix V3: Added adjustableSender modifier. + // Without this modifier, when swapExactInput is called via ERC-223 tokenReceived, + // the delegatecalled swap() in pool_lib reads swap_sender for ERC-223 deposit deduction. + // However, swapExactInput did not set swap_sender, so the pool_lib would attempt to + // call uniswapV3SwapCallback on address(0) (the default swap_sender), which reverts. + // This effectively made swapExactInput unusable for ERC-223 token deposits. function swapExactInput( address recipient, bool zeroForOne, @@ -508,7 +569,7 @@ contract Dex223Pool is IUniswapV3Pool, NoDelegateCall, PeripheryValidation { bytes memory data, uint256 deadline, bool unwrapETH - ) external virtual lock checkDeadline(deadline) returns (uint256 amountOut) { + ) external virtual lock adjustableSender checkDeadline(deadline) returns (uint256 amountOut) { (bool success, bytes memory retdata) = pool_lib.delegatecall( abi.encodeWithSignature("swap(address,bool,int256,uint160,bool,bytes)", unwrapETH ? address(this) : recipient, zeroForOne, amountSpecified, sqrtPriceLimitX96, @@ -601,8 +662,15 @@ contract Dex223Pool is IUniswapV3Pool, NoDelegateCall, PeripheryValidation { return abi.decode(retdata, (uint128, uint128)); } + // @audit-fix V6: Replace .transfer() with .call{value:}(). + // .transfer() forwards only 2300 gas, which is insufficient for contracts + // with receive()/fallback() functions that perform SSTORE or other logic + // (e.g. multisig wallets, proxies). This causes unexpected reverts when the + // factory owner is a contract. Using .call{value:} forwards all available gas. + // Also added zero-address check for _to to prevent burning ETH. function withdrawEther(address payable _to, uint256 _amount) external lock onlyFactoryOwner { - // Transfer the requested amount of Ether - _to.transfer(_amount); + require(_to != address(0), "POOL: ZERO_RECIPIENT"); + (bool success, ) = _to.call{value: _amount}(""); + require(success, "POOL: ETH_TRANSFER_FAILED"); } } diff --git a/contracts/dex-core/Dex223PoolDeployer.sol b/contracts/dex-core/Dex223PoolDeployer.sol index b523713..d1d6ba7 100644 --- a/contracts/dex-core/Dex223PoolDeployer.sol +++ b/contracts/dex-core/Dex223PoolDeployer.sol @@ -5,7 +5,17 @@ import './interfaces/IDex223PoolDeployer.sol'; import './Dex223Pool.sol'; -contract UniswapV3PoolDeployer is IDex223PoolDeployer { +/// @title Dex223 Pool Deployer +/// @notice Deploys Dex223 pools using CREATE2 with a deterministic address derived from token pair and fee. +/// @dev Uses the transient-parameter pattern: parameters are set in storage before the CREATE2 deployment +/// so the child pool constructor can read them, then cleared immediately after deployment. +/// This avoids constructor arguments, keeping the pool init-code hash constant for cheap on-chain +/// address computation. +// @audit-fix V-DEPLOYER-05: Renamed contract from UniswapV3PoolDeployer to Dex223PoolDeployer +// to match the file name and the Dex223 branding. The old name was a legacy artifact from the +// Uniswap V3 fork and caused confusion, since the contract deploys Dex223Pool instances, not +// Uniswap V3 pools. +contract Dex223PoolDeployer is IDex223PoolDeployer { struct Parameters { address factory; address token0_erc20; @@ -17,15 +27,29 @@ contract UniswapV3PoolDeployer is IDex223PoolDeployer { /// @inheritdoc IDex223PoolDeployer Parameters public override parameters; -/* /// @dev Deploys a pool with the given parameters by transiently setting the parameters storage slot and then /// clearing it after deploying the pool. - /// @param factory The contract address of the Uniswap V3 factory - /// @param token0 The first token of the pool by address sort order - /// @param token1 The second token of the pool by address sort order + /// @param factory The contract address of the Dex223 factory + /// @param token0_erc20 The first token of the pool by address sort order (ERC-20 version) + /// @param token1_erc20 The second token of the pool by address sort order (ERC-20 version) /// @param fee The fee collected upon every swap in the pool, denominated in hundredths of a bip /// @param tickSpacing The spacing between usable ticks -*/ + // @audit-fix V-DEPLOYER-02: Added zero-address validation for factory, token0, and token1. + // Without these checks, a misconfigured factory could deploy a pool pointing to address(0) + // for its factory reference or one of its tokens. Such a pool would be permanently broken: + // - factory == address(0): onlyFactoryOwner modifier would call IDex223Factory(address(0)).owner() + // which would revert, locking out all owner-gated functions (setFeeProtocol, collectProtocol, + // withdrawEther). + // - token0/token1 == address(0): balance0()/balance1() would staticcall balanceOf on address(0), + // returning success with 0 balance, making the pool unable to track deposited tokens. + // The CREATE2 salt would "consume" that token-pair+fee combination, permanently preventing + // a correct pool from being deployed for those parameters. + // + // @audit-fix V-DEPLOYER-03: Added check that token0 != token1. + // If both token addresses are identical, the pool would have a single asset pretending to be + // a pair, breaking all swap math (zeroForOne would be meaningless). While the factory also + // checks this, defense-in-depth requires the deployer to enforce the invariant independently + // since it is the contract that actually executes the CREATE2. function deploy( address factory, address token0_erc20, @@ -33,6 +57,11 @@ contract UniswapV3PoolDeployer is IDex223PoolDeployer { uint24 fee, int24 tickSpacing ) internal returns (address pool) { + require(factory != address(0), "DEPLOYER: ZERO_FACTORY"); + require(token0_erc20 != address(0), "DEPLOYER: ZERO_TOKEN0"); + require(token1_erc20 != address(0), "DEPLOYER: ZERO_TOKEN1"); + require(token0_erc20 != token1_erc20, "DEPLOYER: IDENTICAL_TOKENS"); + parameters = Parameters({factory: factory, token0_erc20: token0_erc20, token1_erc20: token1_erc20, fee: fee, tickSpacing: tickSpacing}); pool = address(new Dex223Pool{salt: keccak256(abi.encode(token0_erc20, token1_erc20, fee))}()); delete parameters; diff --git a/contracts/dex-core/Dex223PoolLib.sol b/contracts/dex-core/Dex223PoolLib.sol index 4cff1ab..858f3ac 100644 --- a/contracts/dex-core/Dex223PoolLib.sol +++ b/contracts/dex-core/Dex223PoolLib.sol @@ -13,7 +13,8 @@ import '../libraries/SqrtPriceMath.sol'; import '../libraries/Position.sol'; import '../libraries/LowGasSafeMath.sol'; import '../libraries/SafeCast.sol'; -import '../libraries/SafeERC20Limited.sol'; +// @audit-fix V-LIB-04: Removed SafeERC20Limited import — safeIncreaseAllowance was replaced +// with TransferHelper.safeApprove to avoid uint256 overflow on repeated approvals. import '../libraries/Tick.sol'; import '../libraries/TickBitmap.sol'; import '../libraries/Oracle.sol'; @@ -147,6 +148,22 @@ contract Dex223PoolLib { int24 tick ); + // @audit-note V-LIB-01: Reentrancy protection analysis. + // This contract is a logic library called exclusively via delegatecall from Dex223Pool. + // The Pool contract's `lock` modifier already sets slot0.unlocked = false before + // delegatecalling any PoolLib function. Adding a `lock` modifier here would BREAK + // the delegatecall pattern: the Pool's lock sets unlocked = false, then the PoolLib's + // lock would check `require(slot0.unlocked, 'LOK')` and revert because it reads the + // Pool's storage (where unlocked is already false). + // + // Direct calls to this contract operate on the PoolLib's own storage, which: + // - holds no user funds (the Pool holds funds) + // - has no initialized state (no one calls initialize() on the PoolLib) + // - has no tokens to steal + // Therefore, direct-call reentrancy on the PoolLib is a non-issue. + // + // The reentrancy protection responsibility lies entirely with Dex223Pool.sol. + /// @dev Common checks for valid tick inputs. function checkTicks(int24 tickLower, int24 tickUpper) private pure { require(tickLower < tickUpper, 'TLU'); @@ -342,14 +359,15 @@ contract Dex223PoolLib { } } - /// @dev noDelegateCall is applied indirectly via _modifyPosition + // @audit-note V-LIB-02a: Reentrancy for mint is guarded by Pool's `lock` modifier. + // See V-LIB-01 note above for why we do NOT add a `lock` modifier here. function mint( address recipient, int24 tickLower, int24 tickUpper, uint128 amount, bytes calldata data - ) external /*adjustableSender*/ returns (uint256 amount0, uint256 amount1) { + ) external returns (uint256 amount0, uint256 amount1) { require(amount > 0); (, int256 amount0Int, int256 amount1Int) = _modifyPosition( @@ -375,8 +393,29 @@ contract Dex223PoolLib { emit Mint(msg.sender, recipient, tickLower, tickUpper, amount, amount0, amount1); } + // @audit-fix V-LIB-03: Added underflow protection in conversion path. + // When the initial transfer fails and we fall through to the conversion path, + // the code computes `_amount - _balance` to determine how much to convert. + // If `_balance >= _amount` (e.g. the transfer failed for a reason other than + // insufficient balance, such as a paused token), this subtraction underflows + // in Solidity 0.7.6 (no built-in overflow checks), producing a huge value that + // would drain the converter or revert with a confusing error. + // Fix: require `_amount > _balance` before computing the deficit. + // + // @audit-fix V-LIB-04: Replaced safeIncreaseAllowance with safeApprove(0) + safeApprove(max). + // `safeIncreaseAllowance(token, spender, 2**256 - 1)` computes + // `newAllowance = currentAllowance + (2**256 - 1)` which overflows if + // currentAllowance > 0 (which it will be after the first approval). + // The standard pattern for "approve max once" is to reset to 0 first (to + // handle tokens like USDT that require allowance == 0 before setting a new value), + // then approve the maximum. + // + // @audit-fix V-LIB-09a: Added recipient zero-address check. + // Delivering tokens to address(0) would burn them permanently. function optimisticDelivery(address _token, address _recipient, uint256 _amount) internal { + require(_recipient != address(0), "LIB: ZERO_RECIPIENT"); + bool _is223 = false; if(_token == token0.erc223 || _token == token1.erc223) _is223 = true; // Transfer the tokens and hope that the transfer will succeed i.e. there were @@ -393,30 +432,38 @@ contract Dex223PoolLib { { // NOTE can not get balance if no contract deployed uint _balance = tokenNotExist ? 0 : IERC20Minimal(_token).balanceOf(address(this)); + require(_amount > _balance, "LIB: NO_DEFICIT"); + uint256 _deficit = _amount - _balance; if(_is223) { // take ERC20 version of token address _token20 = (_token == token0.erc223) ? token0.erc20 : token1.erc20; - // Approve the converter first if necessary. - // This approval is expected to execute once and forever. + // Approve the converter if the current allowance is insufficient. if(IERC20Minimal(_token20).allowance(address(this), address(converter)) < _amount) { - //IERC20Minimal(_token20).approve(address(converter), 2**256-1); - SafeERC20.safeIncreaseAllowance(IERC20Minimal(_token20), address(converter), 2**256 - 1); + TransferHelper.safeApprove(_token20, address(converter), 0); + TransferHelper.safeApprove(_token20, address(converter), uint256(-1)); } - converter.convertERC20(_token20, _amount - _balance); + converter.convertERC20(_token20, _deficit); } else { // take ERC223 version of token address _token223 = (_token == token0.erc20) ? token0.erc223 : token1.erc223; - IERC20Minimal(_token223).transfer(address(converter), _amount - _balance); + // @audit-fix V-LIB-10: Use safeTransfer instead of raw transfer. + // Raw transfer() ignores the return value. If the ERC-223 token + // returns false on failure (instead of reverting), the conversion + // silently fails and the subsequent safeTransfer of the output + // token will revert with a confusing "ST" error. + TransferHelper.safeTransfer(_token223, address(converter), _deficit); } TransferHelper.safeTransfer(_token, _recipient, _amount); } } + // @audit-note V-LIB-05a: Reentrancy for collect is guarded by Pool's `lock` modifier. + // See V-LIB-01 note above. function collect( address recipient, int24 tickLower, @@ -448,6 +495,8 @@ contract Dex223PoolLib { emit Collect(msg.sender, recipient, tickLower, tickUpper, amount0, amount1); } + // @audit-note V-LIB-05b: Reentrancy for collectProtocol is guarded by Pool's `lock` modifier. + // See V-LIB-01 note above. function collectProtocol( address recipient, uint128 amount0Requested, @@ -476,12 +525,13 @@ contract Dex223PoolLib { emit CollectProtocol(msg.sender, recipient, amount0, amount1); } - /// @dev noDelegateCall is applied indirectly via _modifyPosition + // @audit-note V-LIB-02b: Reentrancy for burn is guarded by Pool's `lock` modifier. + // See V-LIB-01 note above. function burn( int24 tickLower, int24 tickUpper, uint128 amount - ) external returns (uint256 amount0, uint256 amount1) { + ) external returns (uint256 amount0, uint256 amount1) { (Position.Info storage position, int256 amount0Int, int256 amount1Int) = _modifyPosition( ModifyPositionParams({ @@ -556,6 +606,10 @@ contract Dex223PoolLib { uint256 feeAmount; } + // @audit-note V-LIB-02c: Reentrancy for swap is guarded by Pool's `lock` modifier. + // See V-LIB-01 note above. The swap function is the most critical path. + // Note: noDelegateCall is intentionally omitted because this method IS called + // via delegatecall from the pool, and also indirectly via tokenReceived -> delegatecall. function swap( address recipient, bool zeroForOne, @@ -563,8 +617,7 @@ contract Dex223PoolLib { uint160 sqrtPriceLimitX96, bool prefer223Out, bytes memory data - ) external /*noDelegateCall*/ // noDelegateCall will not prevent delegatecalling - // this method from the same contract via `tokenReceived` of ERC-223 + ) external /*noDelegateCall*/ returns (int256 amount0, int256 amount1) { require(amountSpecified != 0, 'AS'); diff --git a/contracts/dex-core/Dex223QuoteLib.sol b/contracts/dex-core/Dex223QuoteLib.sol index e6ecbb1..82b0b74 100644 --- a/contracts/dex-core/Dex223QuoteLib.sol +++ b/contracts/dex-core/Dex223QuoteLib.sol @@ -13,7 +13,9 @@ import '../libraries/SqrtPriceMath.sol'; import '../libraries/Position.sol'; import '../libraries/LowGasSafeMath.sol'; import '../libraries/SafeCast.sol'; -import '../libraries/SafeERC20Limited.sol'; +// @audit-fix V-QL-05: Removed SafeERC20Limited import — safeIncreaseAllowance was replaced +// with TransferHelper.safeApprove to avoid uint256 overflow on repeated approvals. +// This is consistent with the same fix applied in Dex223PoolLib.sol (V-LIB-04). import '../libraries/Tick.sol'; import '../libraries/TickBitmap.sol'; import '../libraries/Oracle.sol'; @@ -342,8 +344,32 @@ contract Dex223QuoteLib { } } + // @audit-fix V-QL-02: Replaced safeIncreaseAllowance with safeApprove(0) + safeApprove(max). + // `safeIncreaseAllowance(token, spender, 2**256 - 1)` computes + // `newAllowance = currentAllowance + (2**256 - 1)` which overflows if + // currentAllowance > 0 (in Solidity 0.7.6 this wraps silently, producing a + // near-zero allowance). The standard pattern for "approve max once" is to reset + // to 0 first (also handles tokens like USDT that require allowance == 0 before + // setting a new value), then approve the maximum. + // + // @audit-fix V-QL-03: Added underflow protection in conversion path. + // When the initial transfer fails and we fall through to the conversion path, + // `_amount - _balance` can underflow in Solidity 0.7.6 (no built-in overflow + // checks) if `_balance >= _amount` (e.g., transfer failed due to a paused token + // rather than insufficient balance), producing a huge value that would drain the + // converter or revert with a confusing error. + // Fix: require `_amount > _balance` before computing the deficit. + // + // @audit-fix V-QL-04: Added recipient zero-address check. + // Delivering tokens to address(0) would burn them permanently. + // + // @audit-fix V-QL-05: Use TransferHelper.safeTransfer for ERC-223 fallback path. + // Raw transfer() ignores the return value. If the ERC-223 token returns false + // on failure (instead of reverting), the conversion silently fails. function optimisticDelivery(address _token, address _recipient, uint256 _amount) internal { + require(_recipient != address(0), "QLIB: ZERO_RECIPIENT"); + bool _is223 = false; if(_token == token0.erc223 || _token == token1.erc223) _is223 = true; // Transfer the tokens and hope that the transfer will succeed i.e. there were @@ -360,25 +386,26 @@ contract Dex223QuoteLib { { // NOTE can not get balance if no contract deployed uint _balance = tokenNotExist ? 0 : IERC20Minimal(_token).balanceOf(address(this)); + require(_amount > _balance, "QLIB: NO_DEFICIT"); + uint256 _deficit = _amount - _balance; if(_is223) { // take ERC20 version of token address _token20 = (_token == token0.erc223) ? token0.erc20 : token1.erc20; - // Approve the converter first if necessary. - // This approval is expected to execute once and forever. + // Approve the converter if the current allowance is insufficient. if(IERC20Minimal(_token20).allowance(address(this), address(converter)) < _amount) { - //IERC20Minimal(_token20).approve(address(converter), 2**256-1); - SafeERC20.safeIncreaseAllowance(IERC20Minimal(_token20), address(converter), 2**256 - 1); + TransferHelper.safeApprove(_token20, address(converter), 0); + TransferHelper.safeApprove(_token20, address(converter), uint256(-1)); } - converter.convertERC20(_token20, _amount - _balance); + converter.convertERC20(_token20, _deficit); } else { // take ERC223 version of token address _token223 = (_token == token0.erc20) ? token0.erc223 : token1.erc223; - IERC20Minimal(_token223).transfer(address(converter), _amount - _balance); + TransferHelper.safeTransfer(_token223, address(converter), _deficit); } TransferHelper.safeTransfer(_token, _recipient, _amount); } @@ -434,6 +461,32 @@ contract Dex223QuoteLib { uint256 feeAmount; } + // @audit-note V-QL-01: State mutation + revert pattern analysis. + // This function intentionally mutates state (slot0, liquidity, fees, ticks, + // observations) during swap simulation, then reverts via inline assembly to + // return the computed amounts. When called via delegatecall from Dex223Pool, + // the revert rolls back ALL state changes within the delegatecall frame. + // The Pool's quoteSwap() catches the revert data and decodes it. + // + // This is safe as long as: + // (a) quoteSwap always terminates with a revert (guaranteed by the assembly blocks), and + // (b) the function is only invoked via delegatecall (the Pool enforces this). + // + // Direct calls to this contract's quoteSwap operate on the QuoteLib's own + // (uninitialized) storage and always revert, so they are harmless. + // + // @audit-note V-QL-06: Dead code analysis. + // Functions _modifyPosition, _updatePosition, balance0, balance1, checkTicks, + // and _blockTimestamp are defined but only used internally by quoteSwap's swap + // loop logic. They are not dead code — they are called indirectly through the + // swap simulation. However, optimisticDelivery IS dead code in the context of + // quoteSwap (the function reverts before reaching any token delivery). It is + // retained for storage layout consistency with Dex223PoolLib. + // + // @audit-note V-QL-07: No access control (informational). + // Anyone can call quoteSwap directly on this contract. This is safe because: + // (1) the function always reverts, so no state persists, and + // (2) direct calls operate on the QuoteLib's own uninitialized storage. function quoteSwap( address recipient, bool zeroForOne, diff --git a/contracts/test/MickTimeDex223PoolLib.sol b/contracts/test/MickTimeDex223PoolLib.sol index 9fc0613..fe694cf 100644 --- a/contracts/test/MickTimeDex223PoolLib.sol +++ b/contracts/test/MickTimeDex223PoolLib.sol @@ -5,7 +5,6 @@ import '../dex-core/Dex223PoolLib.sol'; // used for testing time dependent behavior contract MockTimeDex223PoolLib is Dex223PoolLib { - address public pool_lib; // Monday, October 5, 2020 9:00:00 AM GMT-05:00 uint256 public time = 1601906400; diff --git a/contracts/test/MockTimeDex223Pool.sol b/contracts/test/MockTimeDex223Pool.sol index e1802b4..09e2511 100644 --- a/contracts/test/MockTimeDex223Pool.sol +++ b/contracts/test/MockTimeDex223Pool.sol @@ -45,6 +45,14 @@ contract MockTimeDex223Pool is Dex223Pool { } } + function _mockUnwrapWETH9(address _recipient, address _weth, uint256 _amountOut) private { + require(_amountOut > 0, "POOL: ZERO_UNWRAP"); + uint256 bal = IWETH9(_weth).balanceOf(address(this)); + require(bal >= _amountOut, 'Insufficient WETH9'); + IWETH9(_weth).withdraw(_amountOut); + TransferHelper.safeTransferETH(_recipient, _amountOut); + } + function swapExactInput( address recipient, bool zeroForOne, @@ -68,7 +76,7 @@ contract MockTimeDex223Pool is Dex223Pool { amountOut = uint256(-(zeroForOne ? amount1 : amount0)); if (unwrapETH) { - unwrapWETH9(recipient, zeroForOne ? token1.erc20 : token0.erc20, amountOutMinimum); + _mockUnwrapWETH9(recipient, zeroForOne ? token1.erc20 : token0.erc20, amountOut); } require(amountOut >= amountOutMinimum, 'Too little received'); @@ -78,8 +86,6 @@ contract MockTimeDex223Pool is Dex223Pool { revert(add(32, retdata), mload(retdata)) } } - - } function setFeeGrowthGlobal0X128(uint256 _feeGrowthGlobal0X128) external { diff --git a/hardhat.config.ts b/hardhat.config.ts index 0e81066..3d382b1 100644 --- a/hardhat.config.ts +++ b/hardhat.config.ts @@ -123,6 +123,24 @@ const config: HardhatUserConfig = { } } }, + "contracts/dex-periphery/Revenue_old.sol": { + version: "0.8.19", + settings: { + optimizer: { + enabled: true, + runs: 5000, + } + } + }, + "contracts/dex-periphery/RevenueV1.sol": { + version: "0.8.19", + settings: { + optimizer: { + enabled: true, + runs: 5000, + } + } + }, } },