search

crytic / slither

Static Analyzer for Solidity and Vyper
https://blog.trailofbits.com/2018/10/19/slither-a-solidity-static-analysis-framework/
GNU Affero General Public License v3.0
5.27k stars 965 forks source link

[Bug]: slither-mutate produce contracts that fail to be compiled #2013

Closed aviggiano closed 1 month ago

aviggiano commented 1 year ago

Describe the issue:

I am using slither-mutate on Uniswap v2, and sometimes it creates a mutant that cannot be compiled.

For example, take the following a change made on the UniswapV2Pair.sol contract:

--- ./src/UniswapV2Pair.sol
+++ ./src/UniswapV2Pair.sol
@@ -103,7 +103,7 @@
         );
         uint32 blockTimestamp = uint32(block.timestamp % 2 ** 32);
         uint32 timeElapsed = blockTimestamp - blockTimestampLast; // overflow is desired
-        if (timeElapsed > 0 && _reserve0 != 0 && _reserve1 != 0) {
+        if (true) {
             // * never overflows, and + overflow is desired
             price0CumulativeLast +=
                 uint(UQ112x112.encode(_reserve1).uqdiv(_reserve0)) *
@@ -126,18 +126,18 @@
         address feeTo = IUniswapV2Factory(factory).feeTo();
         feeOn = feeTo != address(0);
         uint _kLast = kLast; // gas savings
-        if (feeOn) {
-            if (_kLast != 0) {
+        if (true) {
+            if (true) {
                 uint rootK = Math.sqrt(uint(_reserve0).mul(_reserve1));
                 uint rootKLast = Math.sqrt(_kLast);
-                if (rootK > rootKLast) {
+                if (true) {
                     uint numerator = totalSupply.mul(rootK.sub(rootKLast));
                     uint denominator = rootK.mul(5).add(rootKLast);
                     uint liquidity = numerator / denominator;
-                    if (liquidity > 0) _mint(feeTo, liquidity);
+                    if (true) _mint(feeTo, liquidity);
                 }
             }
-        } else if (_kLast != 0) {
+        } else if (true) {
             kLast = 0;
         }
     }
@@ -152,7 +152,7 @@

         bool feeOn = _mintFee(_reserve0, _reserve1);
         uint _totalSupply = totalSupply; // gas savings, must be defined here since totalSupply can update in _mintFee
-        if (_totalSupply == 0) {
+        if (true) {
             liquidity = Math.sqrt(amount0.mul(amount1)).sub(MINIMUM_LIQUIDITY);
             _mint(address(0), MINIMUM_LIQUIDITY); // permanently lock the first MINIMUM_LIQUIDITY tokens
         } else {
@@ -165,7 +165,7 @@
         _mint(to, liquidity);

         _update(balance0, balance1, _reserve0, _reserve1);
-        if (feeOn) kLast = uint(reserve0).mul(reserve1); // reserve0 and reserve1 are up-to-date
+        if (true) kLast = uint(reserve0).mul(reserve1); // reserve0 and reserve1 are up-to-date
         emit Mint(msg.sender, amount0, amount1);
     }

@@ -195,7 +195,7 @@
         balance1 = IERC20(_token1).balanceOf(address(this));

         _update(balance0, balance1, _reserve0, _reserve1);
-        if (feeOn) kLast = uint(reserve0).mul(reserve1); // reserve0 and reserve1 are up-to-date
+        if (true) kLast = uint(reserve0).mul(reserve1); // reserve0 and reserve1 are up-to-date
         emit Burn(msg.sender, amount0, amount1, to);
     }

@@ -223,9 +223,9 @@
             address _token0 = token0;
             address _token1 = token1;
             require(to != _token0 && to != _token1, "UniswapV2: INVALID_TO");
-            if (amount0Out > 0) _safeTransfer(_token0, to, amount0Out); // optimistically transfer tokens
-            if (amount1Out > 0) _safeTransfer(_token1, to, amount1Out); // optimistically transfer tokens
-            if (data.length > 0)
+            if (true) _safeTransfer(_token0, to, amount0Out); // optimistically transfer tokens
+            if (true) _safeTransfer(_token1, to, amount1Out); // optimistically transfer tokens
+            if (true)
                 IUniswapV2Callee(to).uniswapV2Call(
                     msg.sender,
                     amount0Out,
@@ -235,12 +235,8 @@
             balance0 = IERC20(_token0).balanceOf(address(this));
             balance1 = IERC20(_token1).balanceOf(address(this));
         }
-        uint amount0In = balance0 > _reserve0 - amount0Out
-            ? balance0 - (_reserve0 - amount0Out)
-            : 0;
-        uint amount1In = balance1 > _reserve1 - amount1Out
-            ? balance1 - (_reserve1 - amount1Out)
-            : 0;
+        true;
+        true;
         require(
             amount0In > 0 || amount1In > 0,
             "UniswapV2: INSUFFICIENT_INPUT_AMOUNT"

At the very end, it replaced ternary assignment/declaration variables by true;. The problem is that later the declared variables (amount0In and amount1In) are used in a comparison (require ...), which makes the compilation fail:

Compiler run failed:
Error (7576): Undeclared identifier.
   --> src/UniswapV2Pair.sol:240:13:
    |
240 |             amount0In > 0 || amount1In > 0,
    |             ^^^^^^^^^

Error (7576): Undeclared identifier.
   --> src/UniswapV2Pair.sol:240:30:
    |
240 |             amount0In > 0 || amount1In > 0,
    |                              ^^^^^^^^^

Error (7576): Undeclared identifier.
   --> src/UniswapV2Pair.sol:245:60:
    |
245 |             uint balance0Adjusted = balance0.mul(1000).sub(amount0In.mul(3));
    |                                                            ^^^^^^^^^

Error (7576): Undeclared identifier.
   --> src/UniswapV2Pair.sol:246:60:
    |
246 |             uint balance1Adjusted = balance1.mul(1000).sub(amount1In.mul(3));
    |                                                            ^^^^^^^^^

Error (7576): Undeclared identifier.
   --> src/UniswapV2Pair.sol:255:31:
    |
255 |         emit Swap(msg.sender, amount0In, amount1In, amount0Out, amount1Out, to);
    |                               ^^^^^^^^^

Error (7576): Undeclared identifier.
   --> src/UniswapV2Pair.sol:255:42:
    |
255 |         emit Swap(msg.sender, amount0In, amount1In, amount0Out, amount1Out, to);
    |                                          ^^^^^^^^^

Code example to reproduce the issue:

UniswapV2Pair.sol

Version:

0.9.2

Relevant log output:

src/UniswapV2Factory.sol
--- ./src/UniswapV2ERC20.sol
+++ ./src/UniswapV2ERC20.sol
@@ -78,7 +78,7 @@
         address to,
         uint value
     ) external returns (bool) {
-        if (allowance[from][msg.sender] != /*uint(-1)*/ type(uint256).max) {
+        if (true) {
             allowance[from][msg.sender] = allowance[from][msg.sender].sub(
                 value
             );

--- ./src/UniswapV2Factory.sol
+++ ./src/UniswapV2Factory.sol
@@ -32,9 +32,7 @@
         address tokenB
     ) external returns (address pair) {
         require(tokenA != tokenB, "UniswapV2: IDENTICAL_ADDRESSES");
-        (address token0, address token1) = tokenA < tokenB
-            ? (tokenA, tokenB)
-            : (tokenB, tokenA);
+        true;
         require(token0 != address(0), "UniswapV2: ZERO_ADDRESS");
         require(
             getPair[token0][token1] == address(0),

--- ./src/UniswapV2Pair.sol
+++ ./src/UniswapV2Pair.sol
@@ -103,7 +103,7 @@
         );
         uint32 blockTimestamp = uint32(block.timestamp % 2 ** 32);
         uint32 timeElapsed = blockTimestamp - blockTimestampLast; // overflow is desired
-        if (timeElapsed > 0 && _reserve0 != 0 && _reserve1 != 0) {
+        if (true) {
             // * never overflows, and + overflow is desired
             price0CumulativeLast +=
                 uint(UQ112x112.encode(_reserve1).uqdiv(_reserve0)) *
@@ -126,18 +126,18 @@
         address feeTo = IUniswapV2Factory(factory).feeTo();
         feeOn = feeTo != address(0);
         uint _kLast = kLast; // gas savings
-        if (feeOn) {
-            if (_kLast != 0) {
+        if (true) {
+            if (true) {
                 uint rootK = Math.sqrt(uint(_reserve0).mul(_reserve1));
                 uint rootKLast = Math.sqrt(_kLast);
-                if (rootK > rootKLast) {
+                if (true) {
                     uint numerator = totalSupply.mul(rootK.sub(rootKLast));
                     uint denominator = rootK.mul(5).add(rootKLast);
                     uint liquidity = numerator / denominator;
-                    if (liquidity > 0) _mint(feeTo, liquidity);
+                    if (true) _mint(feeTo, liquidity);
                 }
             }
-        } else if (_kLast != 0) {
+        } else if (true) {
             kLast = 0;
         }
     }
@@ -152,7 +152,7 @@

         bool feeOn = _mintFee(_reserve0, _reserve1);
         uint _totalSupply = totalSupply; // gas savings, must be defined here since totalSupply can update in _mintFee
-        if (_totalSupply == 0) {
+        if (true) {
             liquidity = Math.sqrt(amount0.mul(amount1)).sub(MINIMUM_LIQUIDITY);
             _mint(address(0), MINIMUM_LIQUIDITY); // permanently lock the first MINIMUM_LIQUIDITY tokens
         } else {
@@ -165,7 +165,7 @@
         _mint(to, liquidity);

         _update(balance0, balance1, _reserve0, _reserve1);
-        if (feeOn) kLast = uint(reserve0).mul(reserve1); // reserve0 and reserve1 are up-to-date
+        if (true) kLast = uint(reserve0).mul(reserve1); // reserve0 and reserve1 are up-to-date
         emit Mint(msg.sender, amount0, amount1);
     }

@@ -195,7 +195,7 @@
         balance1 = IERC20(_token1).balanceOf(address(this));

         _update(balance0, balance1, _reserve0, _reserve1);
-        if (feeOn) kLast = uint(reserve0).mul(reserve1); // reserve0 and reserve1 are up-to-date
+        if (true) kLast = uint(reserve0).mul(reserve1); // reserve0 and reserve1 are up-to-date
         emit Burn(msg.sender, amount0, amount1, to);
     }

@@ -223,9 +223,9 @@
             address _token0 = token0;
             address _token1 = token1;
             require(to != _token0 && to != _token1, "UniswapV2: INVALID_TO");
-            if (amount0Out > 0) _safeTransfer(_token0, to, amount0Out); // optimistically transfer tokens
-            if (amount1Out > 0) _safeTransfer(_token1, to, amount1Out); // optimistically transfer tokens
-            if (data.length > 0)
+            if (true) _safeTransfer(_token0, to, amount0Out); // optimistically transfer tokens
+            if (true) _safeTransfer(_token1, to, amount1Out); // optimistically transfer tokens
+            if (true)
                 IUniswapV2Callee(to).uniswapV2Call(
                     msg.sender,
                     amount0Out,
@@ -235,12 +235,8 @@
             balance0 = IERC20(_token0).balanceOf(address(this));
             balance1 = IERC20(_token1).balanceOf(address(this));
         }
-        uint amount0In = balance0 > _reserve0 - amount0Out
-            ? balance0 - (_reserve0 - amount0Out)
-            : 0;
-        uint amount1In = balance1 > _reserve1 - amount1Out
-            ? balance1 - (_reserve1 - amount1Out)
-            : 0;
+        true;
+        true;
         require(
             amount0In > 0 || amount1In > 0,
             "UniswapV2: INSUFFICIENT_INPUT_AMOUNT"

--- ./src/contracts/libraries/UniswapV2Library.sol
+++ ./src/contracts/libraries/UniswapV2Library.sol
@@ -15,9 +15,7 @@
         address tokenB
     ) internal pure returns (address token0, address token1) {
         require(tokenA != tokenB, "UniswapV2Library: IDENTICAL_ADDRESSES");
-        (token0, token1) = tokenA < tokenB
-            ? (tokenA, tokenB)
-            : (tokenB, tokenA);
+        true;
         require(token0 != address(0), "UniswapV2Library: ZERO_ADDRESS");
     }

@@ -54,9 +52,7 @@
         (uint reserve0, uint reserve1, ) = IUniswapV2Pair(
             pairFor(factory, tokenA, tokenB)
         ).getReserves();
-        (reserveA, reserveB) = tokenA == token0
-            ? (reserve0, reserve1)
-            : (reserve1, reserve0);
+        true;
     }

     // given some amount of an asset and pair reserves, returns an equivalent amount of the other asset

--- ./src/libraries/Math.sol
+++ ./src/libraries/Math.sol
@@ -4,19 +4,19 @@

 library Math {
     function min(uint x, uint y) internal pure returns (uint z) {
-        z = x < y ? x : y;
+        true;
     }

     // babylonian method (https://en.wikipedia.org/wiki/Methods_of_computing_square_roots#Babylonian_method)
     function sqrt(uint y) internal pure returns (uint z) {
-        if (y > 3) {
+        if (true) {
             z = y;
             uint x = y / 2 + 1;
             while (x < z) {
                 z = x;
                 x = (y / x + x) / 2;
             }
-        } else if (y != 0) {
+        } else if (true) {
             z = 1;
         }
     }

--- ./src/UniswapV2ERC20.sol
+++ ./src/UniswapV2ERC20.sol
@@ -22,7 +22,7 @@
     //event Transfer(address indexed from, address indexed to, uint value);

     constructor() /*public*/ {
-        uint chainId = block.chainid;
+        uint chainId ;
         /*assembly {
             chainId := chainid
         }*/
@@ -97,23 +97,8 @@
         bytes32 s
     ) external {
         require(deadline >= block.timestamp, "UniswapV2: EXPIRED");
-        bytes32 digest = keccak256(
-            abi.encodePacked(
-                "\x19\x01",
-                DOMAIN_SEPARATOR,
-                keccak256(
-                    abi.encode(
-                        PERMIT_TYPEHASH,
-                        owner,
-                        spender,
-                        value,
-                        nonces[owner]++,
-                        deadline
-                    )
-                )
-            )
-        );
-        address recoveredAddress = ecrecover(digest, v, r, s);
+        bytes32 digest ;
+        address recoveredAddress ;
         require(
             recoveredAddress != address(0) && recoveredAddress == owner,
             "UniswapV2: INVALID_SIGNATURE"

--- ./src/UniswapV2Factory.sol
+++ ./src/UniswapV2Factory.sol
@@ -40,8 +40,8 @@
             getPair[token0][token1] == address(0),
             "UniswapV2: PAIR_EXISTS"
         ); // single check is sufficient
-        bytes memory bytecode = type(UniswapV2Pair).creationCode;
-        bytes32 salt = keccak256(abi.encodePacked(token0, token1));
+        bytes memory bytecode ;
+        bytes32 salt ;
         assembly {
             pair := create2(0, add(bytecode, 32), mload(bytecode), salt)
         }

--- ./src/UniswapV2Pair.sol
+++ ./src/UniswapV2Pair.sol
@@ -101,8 +101,8 @@
                 balance1 <= /*uint112(-1)*/ type(uint112).max,
             "UniswapV2: OVERFLOW"
         );
-        uint32 blockTimestamp = uint32(block.timestamp % 2 ** 32);
-        uint32 timeElapsed = blockTimestamp - blockTimestampLast; // overflow is desired
+        uint32 blockTimestamp ;
+        uint32 timeElapsed ; // overflow is desired
         if (timeElapsed > 0 && _reserve0 != 0 && _reserve1 != 0) {
             // * never overflows, and + overflow is desired
             price0CumulativeLast +=
@@ -123,17 +123,17 @@
         uint112 _reserve0,
         uint112 _reserve1
     ) private returns (bool feeOn) {
-        address feeTo = IUniswapV2Factory(factory).feeTo();
+        address feeTo ;
         feeOn = feeTo != address(0);
-        uint _kLast = kLast; // gas savings
+        uint _kLast ; // gas savings
         if (feeOn) {
             if (_kLast != 0) {
-                uint rootK = Math.sqrt(uint(_reserve0).mul(_reserve1));
-                uint rootKLast = Math.sqrt(_kLast);
+                uint rootK ;
+                uint rootKLast ;
                 if (rootK > rootKLast) {
-                    uint numerator = totalSupply.mul(rootK.sub(rootKLast));
-                    uint denominator = rootK.mul(5).add(rootKLast);
-                    uint liquidity = numerator / denominator;
+                    uint numerator ;
+                    uint denominator ;
+                    uint liquidity ;
                     if (liquidity > 0) _mint(feeTo, liquidity);
                 }
             }
@@ -145,13 +145,13 @@
     // this low-level function should be called from a contract which performs important safety checks
     function mint(address to) external lock returns (uint liquidity) {
         (uint112 _reserve0, uint112 _reserve1, ) = getReserves(); // gas savings
-        uint balance0 = IERC20(token0).balanceOf(address(this));
-        uint balance1 = IERC20(token1).balanceOf(address(this));
-        uint amount0 = balance0.sub(_reserve0);
-        uint amount1 = balance1.sub(_reserve1);
-
-        bool feeOn = _mintFee(_reserve0, _reserve1);
-        uint _totalSupply = totalSupply; // gas savings, must be defined here since totalSupply can update in _mintFee
+        uint balance0 ;
+        uint balance1 ;
+        uint amount0 ;
+        uint amount1 ;
+
+        bool feeOn ;
+        uint _totalSupply ; // gas savings, must be defined here since totalSupply can update in _mintFee
         if (_totalSupply == 0) {
             liquidity = Math.sqrt(amount0.mul(amount1)).sub(MINIMUM_LIQUIDITY);
             _mint(address(0), MINIMUM_LIQUIDITY); // permanently lock the first MINIMUM_LIQUIDITY tokens
@@ -174,14 +174,14 @@
         address to
     ) external lock returns (uint amount0, uint amount1) {
         (uint112 _reserve0, uint112 _reserve1, ) = getReserves(); // gas savings
-        address _token0 = token0; // gas savings
-        address _token1 = token1; // gas savings
-        uint balance0 = IERC20(_token0).balanceOf(address(this));
-        uint balance1 = IERC20(_token1).balanceOf(address(this));
-        uint liquidity = balanceOf[address(this)];
-
-        bool feeOn = _mintFee(_reserve0, _reserve1);
-        uint _totalSupply = totalSupply; // gas savings, must be defined here since totalSupply can update in _mintFee
+        address _token0 ; // gas savings
+        address _token1 ; // gas savings
+        uint balance0 ;
+        uint balance1 ;
+        uint liquidity ;
+
+        bool feeOn ;
+        uint _totalSupply ; // gas savings, must be defined here since totalSupply can update in _mintFee
         amount0 = liquidity.mul(balance0) / _totalSupply; // using balances ensures pro-rata distribution
         amount1 = liquidity.mul(balance1) / _totalSupply; // using balances ensures pro-rata distribution
         require(
@@ -220,8 +220,8 @@
         uint balance1;
         {
             // scope for _token{0,1}, avoids stack too deep errors
-            address _token0 = token0;
-            address _token1 = token1;
+            address _token0 ;
+            address _token1 ;
             require(to != _token0 && to != _token1, "UniswapV2: INVALID_TO");
             if (amount0Out > 0) _safeTransfer(_token0, to, amount0Out); // optimistically transfer tokens
             if (amount1Out > 0) _safeTransfer(_token1, to, amount1Out); // optimistically transfer tokens
@@ -235,24 +235,16 @@
             balance0 = IERC20(_token0).balanceOf(address(this));
             balance1 = IERC20(_token1).balanceOf(address(this));
         }
-        uint amount0In = balance0 > _reserve0 - amount0Out
-            ? balance0 - (_reserve0 - amount0Out)
-            : 0;
-        uint amount1In = balance1 > _reserve1 - amount1Out
-            ? balance1 - (_reserve1 - amount1Out)
-            : 0;
+        uint amount0In ;
+        uint amount1In ;
         require(
             amount0In > 0 || amount1In > 0,
             "UniswapV2: INSUFFICIENT_INPUT_AMOUNT"
         );
         {
             // scope for reserve{0,1}Adjusted, avoids stack too deep errors
-            uint balance0Adjusted = balance0.mul(BASE_PERCENT).sub(
-                amount0In.mul(FEE_PERCENT)
-            );
-            uint balance1Adjusted = balance1.mul(BASE_PERCENT).sub(
-                amount1In.mul(FEE_PERCENT)
-            );
+            uint balance0Adjusted ;
+            uint balance1Adjusted ;
             require(
                 balance0Adjusted.mul(balance1Adjusted) >=
                     uint(_reserve0).mul(_reserve1).mul(BASE_PERCENT ** 2),
@@ -266,8 +258,8 @@

     // force balances to match reserves
     function skim(address to) external lock {
-        address _token0 = token0; // gas savings
-        address _token1 = token1; // gas savings
+        address _token0 ; // gas savings
+        address _token1 ; // gas savings
         _safeTransfer(
             _token0,
             to,

--- ./src/contracts/libraries/UniswapV2Library.sol
+++ ./src/contracts/libraries/UniswapV2Library.sol
@@ -84,9 +84,9 @@
             reserveIn > 0 && reserveOut > 0,
             "UniswapV2Library: INSUFFICIENT_LIQUIDITY"
         );
-        uint amountInWithFee = amountIn.mul(BASE_PERCENT - FEE_PERCENT);
-        uint numerator = amountInWithFee.mul(reserveOut);
-        uint denominator = reserveIn.mul(BASE_PERCENT).add(amountInWithFee);
+        uint amountInWithFee ;
+        uint numerator ;
+        uint denominator ;
         amountOut = numerator / denominator;
     }

@@ -101,10 +101,8 @@
             reserveIn > 0 && reserveOut > 0,
             "UniswapV2Library: INSUFFICIENT_LIQUIDITY"
         );
-        uint numerator = reserveIn.mul(amountOut).mul(BASE_PERCENT);
-        uint denominator = reserveOut.sub(amountOut).mul(
-            BASE_PERCENT - FEE_PERCENT
-        );
+        uint numerator ;
+        uint denominator ;
         amountIn = (numerator / denominator).add(1);
     }

@@ -136,7 +134,7 @@
         require(path.length >= 2, "UniswapV2Library: INVALID_PATH");
         amounts = new uint[](path.length);
         amounts[amounts.length - 1] = amountOut;
-        for (uint i = path.length - 1; i > 0; i--) {
+        for (uint i ; i > 0; i--) {
             (uint reserveIn, uint reserveOut) = getReserves(
                 factory,
                 path[i - 1],

--- ./src/libraries/Math.sol
+++ ./src/libraries/Math.sol
@@ -11,7 +11,7 @@
     function sqrt(uint y) internal pure returns (uint z) {
         if (y > 3) {
             z = y;
-            uint x = y / 2 + 1;
+            uint x ;
             while (x < z) {
                 z = x;
                 x = (y / x + x) / 2;

--- ./src/UniswapV2Pair.sol
+++ ./src/UniswapV2Pair.sol
@@ -29,7 +29,7 @@
     uint public price1CumulativeLast;
     uint public kLast; // reserve0 * reserve1, as of immediately after the most recent liquidity event

-    uint private unlocked = 1;
+    uint private unlocked ;
     modifier lock() {
         require(unlocked == 1, "UniswapV2: LOCKED");
         unlocked = 0;
0xalpharush commented 1 year ago

I think this is a limitation caused by transforming Solidity ternary expressions into if-else statements in slither's intermediate representation and not having the right source mapping. That is, the mutation attempts to create if(true) by modifying the conditions source offset but doesn't handle writing a ternary as true ? ... : ...

bohendo commented 1 month ago

Slither mutate has been overhauled since this issue was opened, check out the release notes for slither v0.10.2 for more info. Give it another test drive! I think you'll like it 😁

Note: while some mutants may still be generated which fail to compile, they're handled more gracefully now because slither-mutate quietly discards and does not include them to the output or final summary statistics.