From 710adc32549f651c3e2adb3c67c4e485156c3875 Mon Sep 17 00:00:00 2001 From: Noah Zinsmeister Date: Wed, 6 Nov 2019 17:23:03 -0500 Subject: [PATCH] changes --- README.md | 27 +--- contracts/UniswapV2.sol | 220 +++++++++++++--------------- contracts/UniswapV2Factory.sol | 6 +- contracts/interfaces/IERC20.sol | 12 +- contracts/interfaces/IUniswapV2.sol | 10 +- contracts/libraries/Math.sol | 35 ++--- contracts/libraries/SafeMath128.sol | 33 ++--- contracts/libraries/SafeMath256.sol | 29 ++-- contracts/test/Oracle.sol | 132 ++++++++++------- contracts/token/ERC20.sol | 24 ++- contracts/token/SafeTransfer.sol | 23 +++ test/ERC20.spec.ts | 6 +- test/Oracle.spec.ts | 85 +++++++++-- test/UniswapV2.spec.ts | 47 +++--- test/shared/utilities.ts | 25 ++++ 15 files changed, 399 insertions(+), 315 deletions(-) create mode 100644 contracts/token/SafeTransfer.sol diff --git a/README.md b/README.md index 977d577..a04a510 100644 --- a/README.md +++ b/README.md @@ -25,25 +25,8 @@ yarn test ## Implementation References -### [`contracts/libraries/Math.sol`](./contracts/libraries/Math.sol) - -#### OpenZeppelin -[https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2f9ae975c8bdc5c7f7fa26204896f6c717f07164/contracts/math/Math.sol#L17](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2f9ae975c8bdc5c7f7fa26204896f6c717f07164/contracts/math/Math.sol#L17) - -#### dapp-bin -[https://github.com/ethereum/dapp-bin/pull/50](https://github.com/ethereum/dapp-bin/pull/50) - -[https://github.com/ethereum/dapp-bin/blob/11f05fc9e3f31a00d57982bc2f65ef2654f1b569/library/math.sol#L28](https://github.com/ethereum/dapp-bin/blob/11f05fc9e3f31a00d57982bc2f65ef2654f1b569/library/math.sol#L28) - -### [`contracts/libraries/SafeMath{128,256}.sol`](./contracts/libraries/) - -#### OpenZeppelin -[https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2f9ae975c8bdc5c7f7fa26204896f6c717f07164/contracts/math/SafeMath.sol](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2f9ae975c8bdc5c7f7fa26204896f6c717f07164/contracts/math/SafeMath.sol) - -### [`contracts/implementations/ERC20.sol`](./contracts/implementations/ERC20.sol) - -#### OpenZeppelin -[https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2f9ae975c8bdc5c7f7fa26204896f6c717f07164/contracts/token/ERC20](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2f9ae975c8bdc5c7f7fa26204896f6c717f07164/contracts/token/ERC20) - -#### Dai -[https://github.com/makerdao/dss/blob/b1fdcfc9b2ab7961bf2ce7ab4008bfcec1c73a88/src/dai.sol](https://github.com/makerdao/dss/blob/b1fdcfc9b2ab7961bf2ce7ab4008bfcec1c73a88/src/dai.sol) \ No newline at end of file +- [dapphub math](https://github.com/dapphub/ds-math/blob/de4576712dcf2c5152d16a04e677002d51d46e60/src/math.sol) +- [dapp-bin math](https://github.com/ethereum/dapp-bin/pull/50) +- [OpenZeppelin ECDSA](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/81b1e4810761b088922dbd19a0642873ea581176/contracts/cryptography/ECDSA.sol) +- [DAI token](https://github.com/makerdao/dss/blob/17be7db1c663d8069308c6b78fa5c5f9d71134a3/src/dai.sol) +- [Coinmonks](https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca) diff --git a/contracts/UniswapV2.sol b/contracts/UniswapV2.sol index e43ece9..0fdbb73 100644 --- a/contracts/UniswapV2.sol +++ b/contracts/UniswapV2.sol @@ -1,17 +1,14 @@ pragma solidity 0.5.12; import "./interfaces/IUniswapV2.sol"; -import "./interfaces/IERC20.sol"; -import "./interfaces/IIncompatibleERC20.sol"; import "./libraries/Math.sol"; import "./libraries/SafeMath128.sol"; -import "./libraries/SafeMath256.sol"; import "./token/ERC20.sol"; +import "./token/SafeTransfer.sol"; -contract UniswapV2 is IUniswapV2, ERC20("Uniswap V2", "UNI-V2", 18, 0) { - using Math for uint256; +contract UniswapV2 is IUniswapV2, ERC20("Uniswap V2", "UNI-V2", 18, 0), SafeTransfer { using SafeMath128 for uint128; using SafeMath256 for uint256; @@ -20,20 +17,16 @@ contract UniswapV2 is IUniswapV2, ERC20("Uniswap V2", "UNI-V2", 18, 0) { uint128 token1; } - struct TimeData { - uint64 blockNumber; // overflows >280 billion years after the earth's sun explodes - } - - bool private locked; // reentrancy lock - address public factory; address public token0; address public token1; TokenData private reserves; TokenData private reservesCumulative; - TimeData private lastUpdate; + TokenData private reservesCumulativeOverflows; + uint256 private blockNumberLast; + bool private locked; modifier lock() { require(!locked, "UniswapV2: LOCKED"); locked = true; @@ -48,7 +41,7 @@ contract UniswapV2 is IUniswapV2, ERC20("Uniswap V2", "UNI-V2", 18, 0) { address indexed sender, address indexed recipient, uint256 liquidity, uint128 amountToken0, uint128 amountToken1 ); event Swap( - address indexed sender, address indexed recipient, address input, uint128 amountInput, uint128 amountOutput + address indexed sender, address indexed recipient, address input, uint128 amountToken0, uint128 amountToken1 ); @@ -57,171 +50,164 @@ contract UniswapV2 is IUniswapV2, ERC20("Uniswap V2", "UNI-V2", 18, 0) { } function initialize(address _token0, address _token1, uint256 chainId) external { - require(msg.sender == factory && token0 == address(0) && token1 == token0, "UniswapV2: ALREADY_INITIALIZED"); + require(msg.sender == factory && token0 == address(0) && token0 == token1, "UniswapV2: ALREADY_INITIALIZED"); token0 = _token0; token1 = _token1; initialize(chainId); } - // https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca - function safeTransfer(address token, address to, uint128 value) private returns (bool result) { - IIncompatibleERC20(token).transfer(to, uint256(value)); - assembly { - switch returndatasize() - case 0 { - result := not(0) // for no-bool responses, treat as successful - } - case 32 { - returndatacopy(0, 0, 32) - result := mload(0) // for (presumably) bool responses, return whatever the function did - } - default { - revert(0, 0) // for invalid responses, revert - } - } - } + function getReservesCumulativeAndOverflows() external view returns (uint128, uint128, uint128, uint128) { + require(blockNumberLast > 0, "UniswapV2: NOT_INITIALIZED"); - function getReserves() external view returns (uint128, uint128) { - return (reserves.token0, reserves.token1); - } + TokenData memory reservesCumulativeNext; + TokenData memory reservesCumulativeOverflowsNext; + // replicate the logic in update + if (block.number > blockNumberLast) { + uint128 blocksElapsed = block.number.sub(blockNumberLast).downcast128(); - function getReservesCumulative() external view returns (uint128, uint128) { - uint64 blockNumber = block.number.downcastTo64(); - if (blockNumber == lastUpdate.blockNumber) { - return (reservesCumulative.token0, reservesCumulative.token1); + TokenData memory remaindersMul; + TokenData memory overflowsMul; + (remaindersMul.token0, overflowsMul.token0) = reserves.token0.omul(blocksElapsed); + (remaindersMul.token1, overflowsMul.token1) = reserves.token1.omul(blocksElapsed); + + TokenData memory overflowsAdd; + (reservesCumulativeNext.token0, overflowsAdd.token0) = reservesCumulative.token0.oadd(remaindersMul.token0); + (reservesCumulativeNext.token1, overflowsAdd.token1) = reservesCumulative.token1.oadd(remaindersMul.token1); + + reservesCumulativeOverflowsNext = TokenData({ + token0: reservesCumulativeOverflows.token0.add(overflowsMul.token0.add(overflowsAdd.token0)), + token1: reservesCumulativeOverflows.token1.add(overflowsMul.token1.add(overflowsAdd.token1)) + }); } else { - // replicate the logic in updateReserves - // TODO do we want to make callers pay for our storage updates here instead of this view method? - uint64 blocksElapsed = blockNumber - lastUpdate.blockNumber; - uint128 reservesCumulativeToken0 = reservesCumulative.token0 + (reserves.token0 * blocksElapsed); - uint128 reservesCumulativeToken1 = reservesCumulative.token1 + (reserves.token1 * blocksElapsed); - return (reservesCumulativeToken0, reservesCumulativeToken1); + reservesCumulativeNext = reservesCumulative; + reservesCumulativeOverflowsNext = reservesCumulativeOverflows; } + + return ( + reservesCumulativeNext.token0, + reservesCumulativeNext.token1, + reservesCumulativeOverflowsNext.token0, + reservesCumulativeOverflowsNext.token1 + ); } - function getAmountOutput( - uint128 amountInput, uint128 reserveInput, uint128 reserveOutput - ) public pure returns (uint128 amountOutput) { + function getAmountOutput(uint128 amountInput, uint128 reserveInput, uint128 reserveOutput) + public pure returns (uint128 amountOutput) + { require(amountInput > 0 && reserveInput > 0 && reserveOutput > 0, "UniswapV2: INVALID_VALUE"); - uint256 amountInputWithFee = uint256(amountInput).mul(1000 - 3); // 30 bips for now, TODO think through this later - uint256 numerator = amountInputWithFee.mul(uint256(reserveOutput)); + uint256 amountInputWithFee = uint256(amountInput).mul(1000 - 3); + uint256 numerator = amountInputWithFee.mul(reserveOutput); uint256 denominator = uint256(reserveInput).mul(1000).add(amountInputWithFee); - amountOutput = numerator.div(denominator).downcastTo128(); + amountOutput = (numerator / denominator).downcast128(); } - function updateReserves(TokenData memory reservesNext) private { - uint64 blockNumber = block.number.downcastTo64(); - uint64 blocksElapsed = blockNumber - lastUpdate.blockNumber; + function update(TokenData memory reservesNext) private { + // if any blocks have gone by since the last time this function was called, we have to update + if (block.number > blockNumberLast) { + // make sure that this isn't the first time this function is being called + if (blockNumberLast > 0) { + uint128 blocksElapsed = block.number.sub(blockNumberLast).downcast128(); - if (blocksElapsed > 0) { - // if this isn't the first-ever call to this function, update the accumulators - if (lastUpdate.blockNumber != 0) { - // TODO do edge case math here - reservesCumulative.token0 += reserves.token0 * blocksElapsed; - reservesCumulative.token1 += reserves.token1 * blocksElapsed; + // multiply previous reserves by elapsed blocks in an overflow-safe way + TokenData memory remaindersMul; + TokenData memory overflowsMul; + (remaindersMul.token0, overflowsMul.token0) = reserves.token0.omul(blocksElapsed); + (remaindersMul.token1, overflowsMul.token1) = reserves.token1.omul(blocksElapsed); + + // update cumulative reserves in an overflow-safe way + TokenData memory overflowsAdd; + (reservesCumulative.token0, overflowsAdd.token0) = reservesCumulative.token0.oadd(remaindersMul.token0); + (reservesCumulative.token1, overflowsAdd.token1) = reservesCumulative.token1.oadd(remaindersMul.token1); + + // update cumulative reserves overflows + reservesCumulativeOverflows = TokenData({ + token0: reservesCumulativeOverflows.token0.add(overflowsMul.token0.add(overflowsAdd.token0)), + token1: reservesCumulativeOverflows.token1.add(overflowsMul.token1.add(overflowsAdd.token1)) + }); } - // update last update - lastUpdate.blockNumber = blockNumber; + // update the last block number + blockNumberLast = block.number; } - reserves.token0 = reservesNext.token0; - reserves.token1 = reservesNext.token1; + // update reserves + reserves = reservesNext; } - // TODO sync/merge/donate function? think about the difference between over/under cases - function mintLiquidity(address recipient) external lock returns (uint256 liquidity) { - // get balances TokenData memory balances = TokenData({ - token0: IERC20(token0).balanceOf(address(this)).downcastTo128(), - token1: IERC20(token1).balanceOf(address(this)).downcastTo128() + token0: IERC20(token0).balanceOf(address(this)).downcast128(), + token1: IERC20(token1).balanceOf(address(this)).downcast128() }); - // get amounts sent to be added as liquidity - // TODO this can fail TokenData memory amounts = TokenData({ token0: balances.token0.sub(reserves.token0), token1: balances.token1.sub(reserves.token1) }); if (totalSupply == 0) { - // TODO is this right? - // TODO enforce min amount? - // TODO enforce no remainder? - // TODO does this round the way we want? - liquidity = Math.sqrt(uint256(amounts.token0).mul(uint256(amounts.token1))); + liquidity = Math.sqrt(uint256(amounts.token0).mul(amounts.token1)); } else { - // TODO is this right? - // TODO "donate" or ignore the extra non-min token amount? - // TODO does this round the way we want? liquidity = Math.min( - uint256(amounts.token0).mul(totalSupply).div(uint256(reserves.token0)), - uint256(amounts.token1).mul(totalSupply).div(uint256(reserves.token1)) + uint256(amounts.token0).mul(totalSupply) / reserves.token0, + uint256(amounts.token1).mul(totalSupply) / reserves.token1 ); } mint(recipient, liquidity); - updateReserves(balances); + update(balances); emit LiquidityMinted(msg.sender, recipient, liquidity, amounts.token0, amounts.token1); } function burnLiquidity(address recipient) external lock returns (uint128 amountToken0, uint128 amountToken1) { - // get liquidity sent to be burned - uint256 liquidity = balanceOf[address(this)]; // TODO is this right? + uint256 liquidity = balanceOf[address(this)]; - // require(liquidity > 0, "UniswapV2: ZERO_AMOUNT"); - - // send tokens back - // TODO is this right? - amountToken0 = liquidity.mul(uint256(reserves.token0)).div(totalSupply).downcastTo128(); - amountToken1 = liquidity.mul(uint256(reserves.token1)).div(totalSupply).downcastTo128(); TokenData memory amounts = TokenData({ - token0: amountToken0, - token1: amountToken1 + token0: (amountToken0 = (liquidity.mul(reserves.token0) / totalSupply).downcast128()), + token1: (amountToken1 = (liquidity.mul(reserves.token1) / totalSupply).downcast128()) }); - require(safeTransfer(token0, recipient, amounts.token0), "UniswapV2: TRANSFER_TOKEN0_FAILED"); - require(safeTransfer(token1, recipient, amounts.token1), "UniswapV2: TRANSFER_TOKEN1_FAILED"); + require(amounts.token0 == 0 || safeTransfer(token0, recipient, amounts.token0), "UniswapV2: TRANSFER_0_FAILED"); + require(amounts.token1 == 0 || safeTransfer(token1, recipient, amounts.token1), "UniswapV2: TRANSFER_1_FAILED"); _burn(address(this), liquidity); - - // TODO replace with reserves math? - TokenData memory balances = TokenData({ - token0: IERC20(token0).balanceOf(address(this)).downcastTo128(), - token1: IERC20(token1).balanceOf(address(this)).downcastTo128() - }); - updateReserves(balances); + update(TokenData({ + token0: IERC20(token0).balanceOf(address(this)).downcast128(), + token1: IERC20(token1).balanceOf(address(this)).downcast128() + })); emit LiquidityBurned(msg.sender, recipient, liquidity, amountToken0, amountToken1); } function swap(address input, address recipient) external lock returns (uint128 amountOutput) { - uint128 inputBalance = IERC20(input).balanceOf(address(this)).downcastTo128(); + uint128 balanceInput = IERC20(input).balanceOf(address(this)).downcast128(); - uint128 amountInput; + TokenData memory amounts; TokenData memory balances; if (input == token0) { - amountInput = inputBalance.sub(reserves.token0); - amountOutput = getAmountOutput(amountInput, reserves.token0, reserves.token1); - require(safeTransfer(token1, recipient, amountOutput), "UniswapV2: TRANSFER_FAILED"); - // TODO replace with reserves math? + uint128 amountInput = balanceInput.sub(reserves.token0); + amounts = TokenData({ + token0: amountInput, + token1: (amountOutput = getAmountOutput(amountInput, reserves.token0, reserves.token1)) + }); + require(amounts.token1 == 0 || safeTransfer(token1, recipient, amounts.token1), "UniswapV2: TRANSFER_1_FAILED"); balances = TokenData({ - token0: inputBalance, - token1: IERC20(token1).balanceOf(address(this)).downcastTo128() + token0: balanceInput, + token1: IERC20(token1).balanceOf(address(this)).downcast128() }); } else { require(input == token1, "UniswapV2: INVALID_INPUT"); - - amountInput = inputBalance.sub(reserves.token1); - amountOutput = getAmountOutput(amountInput, reserves.token1, reserves.token0); - require(safeTransfer(token0, recipient, amountOutput), "UniswapV2: TRANSFER_FAILED"); - // TODO replace with reserves math? + uint128 amountInput = balanceInput.sub(reserves.token1); + amounts = TokenData({ + token0: (amountOutput = getAmountOutput(amountInput, reserves.token1, reserves.token0)), + token1: amountInput + }); + require(amounts.token0 == 0 || safeTransfer(token0, recipient, amounts.token0), "UniswapV2: TRANSFER_0_FAILED"); balances = TokenData({ - token0: IERC20(token0).balanceOf(address(this)).downcastTo128(), - token1: inputBalance + token0: IERC20(token0).balanceOf(address(this)).downcast128(), + token1: balanceInput }); } - updateReserves(balances); - emit Swap(msg.sender, recipient, input, amountInput, amountOutput); + update(balances); + emit Swap(msg.sender, recipient, input, amounts.token0, amounts.token1); } } diff --git a/contracts/UniswapV2Factory.sol b/contracts/UniswapV2Factory.sol index 3b89129..c86db4e 100644 --- a/contracts/UniswapV2Factory.sol +++ b/contracts/UniswapV2Factory.sol @@ -57,14 +57,12 @@ contract UniswapV2Factory is IUniswapV2Factory { require(token0ToToken1ToExchange[pair.token0][pair.token1] == address(0), "UniswapV2Factory: EXCHANGE_EXISTS"); bytes memory exchangeBytecodeMemory = exchangeBytecode; - uint256 exchangeBytecodeLength = exchangeBytecode.length; - bytes32 salt = keccak256(abi.encodePacked(pair.token0, pair.token1, chainId)); // TODO include chainId? - // TODO constructor args? + bytes32 salt = keccak256(abi.encodePacked(pair.token0, pair.token1, chainId)); assembly { exchange := create2( 0, add(exchangeBytecodeMemory, 0x20), - exchangeBytecodeLength, + mload(exchangeBytecodeMemory), salt ) } diff --git a/contracts/interfaces/IERC20.sol b/contracts/interfaces/IERC20.sol index a2433a6..4c5a345 100644 --- a/contracts/interfaces/IERC20.sol +++ b/contracts/interfaces/IERC20.sol @@ -18,15 +18,9 @@ interface IERC20 { function burn(uint256 value) external; function approve(address spender, uint256 value) external returns (bool); function approveMeta( - address owner, - address spender, - uint256 value, - uint256 nonce, - uint256 expiration, - uint8 v, - bytes32 r, - bytes32 s - ) external; + address owner, address spender, uint256 value, uint256 nonce, uint256 expiration, uint8 v, bytes32 r, bytes32 s + ) + external; function transferFrom(address from, address to, uint256 value) external returns (bool); function burnFrom(address from, uint256 value) external; } diff --git a/contracts/interfaces/IUniswapV2.sol b/contracts/interfaces/IUniswapV2.sol index f6be4c8..3ab2450 100644 --- a/contracts/interfaces/IUniswapV2.sol +++ b/contracts/interfaces/IUniswapV2.sol @@ -8,19 +8,17 @@ interface IUniswapV2 { address indexed sender, address indexed recipient, uint256 liquidity, uint128 amountToken0, uint128 amountToken1 ); event Swap( - address indexed sender, address indexed recipient, address input, uint128 amountInput, uint128 amountOutput + address indexed sender, address indexed recipient, address input, uint128 amountToken0, uint128 amountToken1 ); function factory() external view returns (address); function token0() external view returns (address); function token1() external view returns (address); - function getReserves() external view returns (uint128, uint128); - function getReservesCumulative() external view returns (uint128, uint128); + function getReservesCumulativeAndOverflows() external view returns (uint128, uint128, uint128, uint128); - function getAmountOutput( - uint128 amountInput, uint128 reserveInput, uint128 reserveOutput - ) external pure returns (uint128 amountOutput); + function getAmountOutput(uint128 amountInput, uint128 reserveInput, uint128 reserveOutput) + external pure returns (uint128 amountOutput); function mintLiquidity(address recipient) external returns (uint256 liquidity); function burnLiquidity(address recipient) external returns (uint128 amountToken0, uint128 amountToken1); diff --git a/contracts/libraries/Math.sol b/contracts/libraries/Math.sol index b42a393..460bd27 100644 --- a/contracts/libraries/Math.sol +++ b/contracts/libraries/Math.sol @@ -1,35 +1,18 @@ pragma solidity 0.5.12; library Math { - // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2f9ae975c8bdc5c7f7fa26204896f6c717f07164/contracts/math/Math.sol#L17 - function min(uint256 a, uint256 b) internal pure returns (uint256) { - return a < b ? a : b; + function min(uint256 x, uint256 y) internal pure returns (uint256 z) { + return x <= y ? x : y; } - // https://github.com/ethereum/dapp-bin/pull/50 - // https://github.com/ethereum/dapp-bin/blob/11f05fc9e3f31a00d57982bc2f65ef2654f1b569/library/math.sol#L28 function sqrt(uint256 x) internal pure returns (uint256 y) { - if (x == 0) { - y = 0; - } else if (x <= 3) { - y = 1; - } else { - y = x; - uint256 z = (x + 1) / 2; - while (z < y) { - y = z; - z = (x / z + z) / 2; - } + if (x == 0) return 0; + else if (x <= 3) return 1; + uint256 z = (x + 1) / 2; + y = x; + while (z < y) { + y = z; + z = (x / z + z) / 2; } } - - function downcastTo64(uint256 a) internal pure returns (uint64) { - require(a <= uint64(-1), "Math: downcast overflow (uint256 to uint64)"); - return uint64(a); - } - - function downcastTo128(uint256 a) internal pure returns (uint128) { - require(a <= uint128(-1), "Math: downcast overflow (uint256 to uint128)"); - return uint128(a); - } } diff --git a/contracts/libraries/SafeMath128.sol b/contracts/libraries/SafeMath128.sol index 19cd001..84c09ea 100644 --- a/contracts/libraries/SafeMath128.sol +++ b/contracts/libraries/SafeMath128.sol @@ -1,25 +1,24 @@ -// https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2f9ae975c8bdc5c7f7fa26204896f6c717f07164/contracts/math/SafeMath.sol pragma solidity 0.5.12; library SafeMath128 { - function add(uint128 a, uint128 b) internal pure returns (uint128 c) { - c = a + b; - require(c >= a, "SafeMath128: addition overflow"); + function add(uint128 x, uint128 y) internal pure returns (uint128 z) { + require((z = x + y) >= x, "ds-math-add-overflow"); + } + function sub(uint128 x, uint128 y) internal pure returns (uint128 z) { + require((z = x - y) <= x, "ds-math-sub-underflow"); + } + function mul(uint128 x, uint128 y) internal pure returns (uint128 z) { + require(y == 0 || (z = x * y) / y == x, "ds-math-mul-overflow"); } - function sub(uint128 a, uint128 b) internal pure returns (uint128) { - require(b <= a, "SafeMath128: subtraction overflow"); - return a - b; + function oadd(uint128 w, uint128 x) internal pure returns (uint128 y, uint128 z) { + uint256 sum = uint256(w) + x; + z = uint128(sum / uint128(-1)); + y = uint128(sum % uint128(-1)); } - - function mul(uint128 a, uint128 b) internal pure returns (uint128 c) { - if (a == 0) return 0; - c = a * b; - require(c / a == b, "SafeMath128: multiplication overflow"); - } - - function div(uint128 a, uint128 b) internal pure returns (uint128) { - require(b > 0, "SafeMath128: division by zero"); - return a / b; + function omul(uint128 w, uint128 x) internal pure returns (uint128 y, uint128 z) { + uint256 product = uint256(w) * x; + z = uint128(product / uint128(-1)); + y = uint128(product % uint128(-1)); } } diff --git a/contracts/libraries/SafeMath256.sol b/contracts/libraries/SafeMath256.sol index 00d7e64..846c07f 100644 --- a/contracts/libraries/SafeMath256.sol +++ b/contracts/libraries/SafeMath256.sol @@ -1,25 +1,18 @@ -// https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2f9ae975c8bdc5c7f7fa26204896f6c717f07164/contracts/math/SafeMath.sol pragma solidity 0.5.12; library SafeMath256 { - function add(uint256 a, uint256 b) internal pure returns (uint256 c) { - c = a + b; - require(c >= a, "SafeMath256: addition overflow"); + function add(uint256 x, uint256 y) internal pure returns (uint256 z) { + require((z = x + y) >= x, "ds-math-add-overflow"); + } + function sub(uint256 x, uint256 y) internal pure returns (uint256 z) { + require((z = x - y) <= x, "ds-math-sub-underflow"); + } + function mul(uint256 x, uint256 y) internal pure returns (uint256 z) { + require(y == 0 || (z = x * y) / y == x, "ds-math-mul-overflow"); } - function sub(uint256 a, uint256 b) internal pure returns (uint256) { - require(b <= a, "SafeMath256: subtraction overflow"); - return a - b; - } - - function mul(uint256 a, uint256 b) internal pure returns (uint256 c) { - if (a == 0) return 0; - c = a * b; - require(c / a == b, "SafeMath256: multiplication overflow"); - } - - function div(uint256 a, uint256 b) internal pure returns (uint256) { - require(b > 0, "SafeMath256: division by zero"); - return a / b; + function downcast128(uint256 y) internal pure returns (uint128 z) { + require(y <= uint128(-1), "downcast-128-overflow"); + z = uint128(y); } } diff --git a/contracts/test/Oracle.sol b/contracts/test/Oracle.sol index fb2aed5..b2c283b 100644 --- a/contracts/test/Oracle.sol +++ b/contracts/test/Oracle.sol @@ -2,10 +2,12 @@ pragma solidity 0.5.12; import "../interfaces/IUniswapV2.sol"; -import "../libraries/Math.sol"; +import "../libraries/SafeMath256.sol"; contract Oracle { - using Math for uint256; + using SafeMath256 for uint256; + + enum OracleStates { NeedsInitialization, NeedsActivation, Active } struct TokenData { uint128 token0; @@ -13,79 +15,105 @@ contract Oracle { } struct TimeData { - uint64 blockNumber; - uint64 blockTimestamp; + uint128 blockNumber; + uint128 blockTimestamp; } address public exchange; - bool public initialized; - uint64 constant period = 1 days; + uint128 constant period = 1 days; + OracleStates private state = OracleStates.NeedsInitialization; TokenData private reservesCumulative; TimeData private lastUpdate; - TokenData private currentPrice; constructor(address _exchange) public { exchange = _exchange; } - function _updateCurrentPrice(TokenData memory averages, uint64 timeElapsed) private { - TokenData memory nextPrice; - if (timeElapsed >= period || (currentPrice.token0 == 0 && currentPrice.token1 == 0)) { - nextPrice = averages; - } else { - nextPrice = TokenData({ - token0: (currentPrice.token0 * (period - timeElapsed) + averages.token0 * timeElapsed) / period, - token1: (currentPrice.token1 * (period - timeElapsed) + averages.token1 * timeElapsed) / period - }); - } - currentPrice = nextPrice; + function getReservesCumulative() private view returns (TokenData memory) { + IUniswapV2 uniswapV2 = IUniswapV2(exchange); + (uint128 reservesCumulativeToken0, uint128 reservesCumulativeToken1,,) = uniswapV2.getReservesCumulativeAndOverflows(); + return TokenData({ + token0: reservesCumulativeToken0, + token1: reservesCumulativeToken1 + }); + } + + function getTimeData() private view returns (TimeData memory) { + return TimeData({ + blockNumber: block.number.downcast128(), + blockTimestamp: block.timestamp.downcast128() + }); } function initialize() external { - require(!initialized, "Oracle: ALREADY_INITIALIZED"); + require(state == OracleStates.NeedsInitialization, "Oracle: DOES_NOT_NEED_INITIALIZATION"); - IUniswapV2 uniswapV2 = IUniswapV2(exchange); - (uint128 reserveCumulativeToken0, uint128 reserveCumulativeToken1) = uniswapV2.getReservesCumulative(); + reservesCumulative = getReservesCumulative(); + lastUpdate = getTimeData(); - reservesCumulative.token0 = reserveCumulativeToken0; - reservesCumulative.token1 = reserveCumulativeToken1; - lastUpdate.blockNumber = block.number.downcastTo64(); - lastUpdate.blockTimestamp = block.timestamp.downcastTo64(); - - initialized = true; + state = OracleStates.NeedsActivation; } - function updateCurrentPrice() external { - require(initialized, "Oracle: UNINITIALIZED"); + function activate() external { + require(state == OracleStates.NeedsActivation, "Oracle: DOES_NOT_NEED_ACTIVATION"); - uint64 blockNumber = block.number.downcastTo64(); - // if we haven't updated this block yet... - if (blockNumber > lastUpdate.blockNumber) { - IUniswapV2 uniswapV2 = IUniswapV2(exchange); - (uint128 reserveCumulativeToken0, uint128 reserveCumulativeToken1) = uniswapV2.getReservesCumulative(); + // get the current time, ensure it's been >=1 blocks since last update, and record the update + TimeData memory currentTime = getTimeData(); + uint128 blocksElapsed = currentTime.blockNumber - lastUpdate.blockNumber; + require(blocksElapsed > 0, "Oracle: INSUFFICIENT_BLOCKS_PASSED"); + lastUpdate = currentTime; - uint128 blocksElapsed = blockNumber - lastUpdate.blockNumber; + // get the current cumulative reserves, calculate the deltas, and record the new values + TokenData memory reservesCumulativeNext = getReservesCumulative(); + TokenData memory deltas = TokenData({ + token0: reservesCumulativeNext.token0 - reservesCumulative.token0, + token1: reservesCumulativeNext.token1 - reservesCumulative.token1 + }); + reservesCumulative = reservesCumulativeNext; - TokenData memory deltas = TokenData({ - token0: reserveCumulativeToken0 - reservesCumulative.token0, - token1: reserveCumulativeToken1 - reservesCumulative.token1 + // get the average price over the period and set it to the current price + currentPrice = TokenData({ + token0: deltas.token0 / blocksElapsed, + token1: deltas.token1 / blocksElapsed + }); + + state = OracleStates.Active; + } + + function update() external { + require(state == OracleStates.Active, "Oracle: INACTIVE"); + + // get the current time, ensure it's been >=1 blocks since last update, and record the update + TimeData memory currentTime = getTimeData(); + uint128 blocksElapsed = currentTime.blockNumber - lastUpdate.blockNumber; + require(blocksElapsed > 0, "Oracle: INSUFFICIENT_BLOCKS_PASSED"); + uint128 timeElapsed = currentTime.blockTimestamp - lastUpdate.blockTimestamp; + lastUpdate = currentTime; + + // get the current cumulative reserves, calculate the deltas, and record the new values + TokenData memory reservesCumulativeNext = getReservesCumulative(); + TokenData memory deltas = TokenData({ + token0: reservesCumulativeNext.token0 - reservesCumulative.token0, + token1: reservesCumulativeNext.token1 - reservesCumulative.token1 + }); + reservesCumulative = reservesCumulativeNext; + + // get the average price over the period + TokenData memory averages = TokenData({ + token0: deltas.token0 / blocksElapsed, + token1: deltas.token1 / blocksElapsed + }); + + // update the current price with this information + if (timeElapsed < period) { + currentPrice = TokenData({ + token0: (currentPrice.token0 * (period - timeElapsed) + averages.token0 * timeElapsed) / period, + token1: (currentPrice.token1 * (period - timeElapsed) + averages.token1 * timeElapsed) / period }); - - TokenData memory averages = TokenData({ - token0: deltas.token0 / blocksElapsed, - token1: deltas.token1 / blocksElapsed - }); - - uint64 blockTimestamp = block.timestamp.downcastTo64(); - uint64 timeElapsed = blockTimestamp - lastUpdate.blockTimestamp; - _updateCurrentPrice(averages, timeElapsed); - - reservesCumulative.token0 = reserveCumulativeToken0; - reservesCumulative.token1 = reserveCumulativeToken1; - lastUpdate.blockNumber = blockNumber; - lastUpdate.blockTimestamp = blockTimestamp; + } else { + currentPrice = averages; } } diff --git a/contracts/token/ERC20.sol b/contracts/token/ERC20.sol index c3689ef..2dbe048 100644 --- a/contracts/token/ERC20.sol +++ b/contracts/token/ERC20.sol @@ -1,5 +1,3 @@ -// https://github.com/makerdao/dss/blob/b1fdcfc9b2ab7961bf2ce7ab4008bfcec1c73a88/src/dai.sol -// https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2f9ae975c8bdc5c7f7fa26204896f6c717f07164/contracts/token/ERC20 pragma solidity 0.5.12; import "../interfaces/IERC20.sol"; @@ -74,19 +72,17 @@ contract ERC20 is IERC20 { } function approveMeta( - address owner, - address spender, - uint256 value, - uint256 nonce, - uint256 expiration, - uint8 v, - bytes32 r, - bytes32 s - ) external { + address owner, address spender, uint256 value, uint256 nonce, uint256 expiration, uint8 v, bytes32 r, bytes32 s + ) + external + { require(chainId != 0, "ERC20: UNINITIALIZED"); require(nonce == nonceFor[owner]++, "ERC20: INVALID_NONCE"); require(expiration > block.timestamp, "ERC20: EXPIRED_SIGNATURE"); + require(v == 27 || v == 28, "ECDSA: INVALID_V"); + require(uint256(s) <= 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0, "ECDSA: INVALID_S"); + bytes32 digest = keccak256(abi.encodePacked( hex'19', hex'00', @@ -95,8 +91,10 @@ contract ERC20 is IERC20 { owner, spender, value, nonce, expiration, chainId )) )); - // TODO add ECDSA checks https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/cryptography/ECDSA.sol - require(owner == ecrecover(digest, v, r, s), "ERC20: INVALID_SIGNATURE"); + + address recoveredAddress = ecrecover(digest, v, r, s); + require(recoveredAddress != address(0), "ERC20: INVALID_RECOVERED_ADDRESS"); + require(owner == recoveredAddress, "ERC20: INVALID_SIGNATURE"); _approve(owner, spender, value); } diff --git a/contracts/token/SafeTransfer.sol b/contracts/token/SafeTransfer.sol new file mode 100644 index 0000000..e037b9e --- /dev/null +++ b/contracts/token/SafeTransfer.sol @@ -0,0 +1,23 @@ +pragma solidity 0.5.12; + +import "../interfaces/IIncompatibleERC20.sol"; + +contract SafeTransfer { + function safeTransfer(address token, address to, uint256 value) internal returns (bool result) { + IIncompatibleERC20(token).transfer(to, value); + + assembly { + switch returndatasize() + case 0 { // if there was no return data, treat the transfer as successful + result := 1 + } + case 0x20 { // if the return data was 32 bytes long, return that value + returndatacopy(0, 0, 0x20) + result := mload(0) + } + default { // revert in all other cases + revert(0, 0) + } + } + } +} \ No newline at end of file diff --git a/test/ERC20.spec.ts b/test/ERC20.spec.ts index 05c22c8..d3964e4 100644 --- a/test/ERC20.spec.ts +++ b/test/ERC20.spec.ts @@ -118,10 +118,8 @@ describe('ERC20', () => { it('transfer:fail', async () => { await expect(token.transfer(other.address, TOKEN_DETAILS.totalSupply.add(1))).to.be.revertedWith( - 'SafeMath256: subtraction overflow' - ) - await expect(token.connect(other).transfer(wallet.address, 1)).to.be.revertedWith( - 'SafeMath256: subtraction overflow' + 'ds-math-sub-underflow' ) + await expect(token.connect(other).transfer(wallet.address, 1)).to.be.revertedWith('ds-math-sub-underflow') }) }) diff --git a/test/Oracle.spec.ts b/test/Oracle.spec.ts index 00608cc..860ed9c 100644 --- a/test/Oracle.spec.ts +++ b/test/Oracle.spec.ts @@ -4,14 +4,22 @@ import { solidity, createMockProvider, getWallets, createFixtureLoader, deployCo import { Contract } from 'ethers' import { BigNumber, bigNumberify } from 'ethers/utils' -import { expandTo18Decimals } from './shared/utilities' +import { expandTo18Decimals, mineBlocks } from './shared/utilities' import { exchangeFixture, ExchangeFixture } from './shared/fixtures' import Oracle from '../build/Oracle.json' +const ONE_DAY = 60 * 60 * 24 + chai.use(solidity) const { expect } = chai +interface OracleSnapshot { + cumulativeReserves: BigNumber[] + blockNumber: number + time: number +} + describe('Oracle', () => { const provider = createMockProvider(path.join(__dirname, '..', 'waffle.json')) const [wallet] = getWallets(provider) @@ -37,9 +45,18 @@ describe('Oracle', () => { await exchange.connect(wallet).mintLiquidity(wallet.address) } - async function swap(token: Contract, amount: BigNumber) { - await token.transfer(exchange.address, amount) - await exchange.connect(wallet).swap(token.address, wallet.address) + async function swap(inputToken: Contract, amount: BigNumber) { + const token0 = await exchange.token0() + const reserves = await exchange.getReserves() + + const inputReserve = inputToken.address === token0 ? reserves[0] : reserves[1] + const outputReserve = inputToken.address === token0 ? reserves[1] : reserves[0] + const outputAmount = await exchange.getAmountOutput(amount, inputReserve, outputReserve) + + await inputToken.transfer(exchange.address, amount) + await exchange.connect(wallet).swap(inputToken.address, wallet.address) + + return outputAmount } it('exchange, getCurrentPrice', async () => { @@ -47,15 +64,63 @@ describe('Oracle', () => { expect(await oracle.getCurrentPrice()).to.deep.eq([0, 0].map(n => bigNumberify(n))) }) + async function getOracleSnapshot(): Promise { + const cumulativeReserves = await exchange.getReservesCumulativeAndOverflows() + const blockNumber = await provider.getBlockNumber() + const time = (await provider.getBlock(blockNumber)).timestamp + + return { + cumulativeReserves, + blockNumber, + time + } + } + + // function getExpectedOraclePrice( + // preSnapshot: OracleSnapshot, + // postSnapshot: OracleSnapshot, + // oldPrice: BigNumber[], + // elapsedTime: number + // ) { + // return 1 + // } + it('updateCurrentPrice', async () => { - const token0Amount = expandTo18Decimals(10) - const token1Amount = expandTo18Decimals(5) - + const token0Amount = expandTo18Decimals(5) + const token1Amount = expandTo18Decimals(10) await addLiquidity(token0Amount, token1Amount) - await oracle.connect(wallet).initialize() - await swap(token0, bigNumberify(1)) - await oracle.connect(wallet).updateCurrentPrice() + await oracle.connect(wallet).initialize() + expect(await oracle.getCurrentPrice()).to.deep.eq([0, 0].map(n => bigNumberify(n))) + + await oracle.connect(wallet).activate() expect(await oracle.getCurrentPrice()).to.deep.eq([token0Amount, token1Amount]) + + const preSwapSnapshot = await getOracleSnapshot() + + const swapAmount = expandTo18Decimals(5) + const expectedToken1Amount = await swap(token0, swapAmount) + const postSwapToken0Amount = token0Amount.add(swapAmount) + const postSwapToken1Amount = token1Amount.sub(expectedToken1Amount) + + const postSwapSnapshot = await getOracleSnapshot() + + const elapsedBlocks = postSwapSnapshot.blockNumber - preSwapSnapshot.blockNumber + expect(elapsedBlocks).to.eq(2) + + await oracle.connect(wallet).update() + + const elapsedTime = postSwapSnapshot.time - preSwapSnapshot.time + if (elapsedTime === 0) { + expect(await oracle.getCurrentPrice()).to.deep.eq([token0Amount, token1Amount]) + } else { + console.log('uh oh!') + // expect(await oracle.getCurrentPrice()).to.deep.eq([token0Amount, token1Amount]) + } + + // console.log((await oracle.getCurrentPrice()).map((p: BigNumber): string => p.toString())) + + // await mineBlocks(provider, 1, timePost + 60 * 60 * (24 / 2)) + // await oracle.connect(wallet).update() }) }) diff --git a/test/UniswapV2.spec.ts b/test/UniswapV2.spec.ts index c360202..878bf72 100644 --- a/test/UniswapV2.spec.ts +++ b/test/UniswapV2.spec.ts @@ -94,7 +94,15 @@ describe('UniswapV2', () => { const expectedOutputAmount = bigNumberify('1662497915624478906') await token0.transfer(exchange.address, swapAmount) - await expect(exchange.connect(wallet).swap(token0.address, wallet.address)) + console.log(await exchange.estimate.swap(token0.address, wallet.address)) + await expect( + exchange + .connect(wallet) + .swap(token0.address, wallet.address) + .then((a: any) => { + console.log(a) + }) + ) .to.emit(exchange, 'Swap') .withArgs(wallet.address, wallet.address, token0.address, swapAmount, expectedOutputAmount) @@ -127,31 +135,36 @@ describe('UniswapV2', () => { expect(await token1.balanceOf(wallet.address)).to.eq(totalSupplyToken1) }) - it('getReserves', async () => { - expect(await exchange.getReserves()).to.deep.eq([0, 0].map(n => bigNumberify(n))) + // it('getReserves', async () => { + // expect(await exchange.getReserves()).to.deep.eq([0, 0].map(n => bigNumberify(n))) + + // const token0Amount = expandTo18Decimals(3) + // const token1Amount = expandTo18Decimals(3) + // await addLiquidity(token0Amount, token1Amount) + + // expect(await exchange.getReserves()).to.deep.eq([token0Amount, token1Amount]) + // }) + + it('getReservesCumulativeAndOverflows', async () => { + expect(await exchange.getReservesCumulativeAndOverflows()).to.deep.eq([0, 0, 0, 0].map(n => bigNumberify(n))) const token0Amount = expandTo18Decimals(3) const token1Amount = expandTo18Decimals(3) await addLiquidity(token0Amount, token1Amount) - expect(await exchange.getReserves()).to.deep.eq([token0Amount, token1Amount]) - }) - - it('getReservesCumulative', async () => { - expect(await exchange.getReservesCumulative()).to.deep.eq([0, 0].map(n => bigNumberify(n))) - - const token0Amount = expandTo18Decimals(3) - const token1Amount = expandTo18Decimals(3) - await addLiquidity(token0Amount, token1Amount) - - const reservesCumulativePre = await exchange.getReservesCumulative() - expect(reservesCumulativePre).to.deep.eq([0, 0].map(n => bigNumberify(n))) + const reservesCumulativePre = await exchange.getReservesCumulativeAndOverflows() + expect(reservesCumulativePre).to.deep.eq([0, 0, 0, 0].map(n => bigNumberify(n))) const dummySwapAmount = bigNumberify(1) await token0.transfer(exchange.address, dummySwapAmount) await exchange.connect(wallet).swap(token0.address, wallet.address) - const reservesCumulativePost = await exchange.getReservesCumulative() - expect(reservesCumulativePost).to.deep.eq([token0Amount.mul(bigNumberify(2)), token1Amount.mul(bigNumberify(2))]) + const reservesCumulativePost = await exchange.getReservesCumulativeAndOverflows() + expect(reservesCumulativePost).to.deep.eq([ + token0Amount.mul(bigNumberify(2)), + token1Amount.mul(bigNumberify(2)), + 0, + 0 + ]) }) }) diff --git a/test/shared/utilities.ts b/test/shared/utilities.ts index da1ece2..0666778 100644 --- a/test/shared/utilities.ts +++ b/test/shared/utilities.ts @@ -1,3 +1,4 @@ +import { providers } from 'ethers' import { BigNumber, bigNumberify, getAddress, keccak256, solidityPack } from 'ethers/utils' import { CHAIN_ID } from './constants' @@ -48,3 +49,27 @@ export function getCreate2Address( return getAddress(`0x${keccak256(sanitizedInputs).slice(-40)}`) } + +async function mineBlock(provider: providers.Web3Provider, timestamp?: number): Promise { + await new Promise((resolve, reject) => { + ;(provider._web3Provider.sendAsync as any)( + { jsonrpc: '2.0', method: 'evm_mine', params: timestamp ? [timestamp] : [] }, + (error: any, result: any): void => { + if (error) { + reject(error) + } else { + resolve(result) + } + } + ) + }) +} + +export async function mineBlocks( + provider: providers.Web3Provider, + numberOfBlocks: number, + timestamp?: number +): Promise { + await Promise.all([...Array(numberOfBlocks - 1)].map(() => mineBlock(provider))) + await mineBlock(provider, timestamp) +}