Skip to content

Commit

Permalink
fix: assembly in getBytecode that resulted in extra data copied
Browse files Browse the repository at this point in the history
* test: getBytecode
* fix(generic-factory): bytecode array was 1 word too long
* test: make sure that bytecode generated and create2 address is correct
* lint: update .prettierignore to exclude broadcast
* lint: forge fmt
* chore: rm broadcast as those txs are no longer valid
* ci: update hashes
* test: update gas snapshot
---------
Co-authored-by: OliverNChalk <11343499+OliverNChalk@users.noreply.github.com>
  • Loading branch information
xenide authored Jun 19, 2023
1 parent 5d3da19 commit 8885123
Show file tree
Hide file tree
Showing 9 changed files with 195 additions and 356 deletions.
148 changes: 75 additions & 73 deletions .gas-snapshot

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions .prettierignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/broadcast
/cache
/out
/node_modules
/lib
/reference

278 changes: 0 additions & 278 deletions broadcast/DeployScript.s.sol/43114/run-1686694128.json

This file was deleted.

2 changes: 1 addition & 1 deletion script/optimized-deployer-meta
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"constant_product_hash": "0xde6d8452b97c785350ce9f82090dfb5962933aa5467827310798925c8725013b",
"factory_hash": "0xda6f297f6ce8cf60c5dcf260b6c4178f82c853ae2f16446c064cf6f4e2e4b1ca",
"factory_hash": "0xaafa0bd4694a22c24f8ef26cfd45864f11be2102e269ee6a831d2c659b30f08f",
"oracle_caller_hash": "0x1dcb29ca6399be1a5f8a4b3f168a83ff20697718c3d5434ddeeb4a8050fdc615",
"stable_hash": "0x1289a8879d4d1992d308d2a4d8457a5ca8d57e07effe092f2a1f21b0b9b10619"
}
2 changes: 1 addition & 1 deletion script/unoptimized-deployer-meta
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"constant_product_hash": "0x71f0d385cb55f83e9dd571b118cb5ee391542d27ff471998ea316d3693b21308",
"factory_hash": "0x055e072b9986c94cc4b0f84188217c205895b41faec500b7aed5300e20b29605",
"factory_hash": "0xa61785bbfa1f668e1ad6e2aff9c6fc4ed961b95ae7cc54c6463d49b259b2e984",
"oracle_caller_hash": "0x6e5b6d511ec5f70fdae7be4b74be5a313df4168f1b50dee9fc571e4414697354",
"stable_hash": "0xa17d9c01054e0d89b5da9d84936c6960c6f82f7981951978cc2aff275d943f5a"
}
11 changes: 9 additions & 2 deletions src/GenericFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,15 @@ contract GenericFactory is IGenericFactory, Owned {
// the difference between free_mem (stack) and mem pointer (memory)
// to know how much memory we just wrote and thus the size of the
// bytecode.
mstore(lInitCode, add(sub(free_mem, mload(0x40)), 0x40))
mstore(0x40, add(free_mem, 0x40))
//
// Because solidity arrays in memory store their length in the first
// word, we get the array size using the following formula:
//
// old_free_memory = mload(0x40)
// new_free_memory = free_mem + 64 (the two tokens)
// array_length = new_free_memory - old_free_memory - 32 (the length word)
mstore(lInitCode, add(sub(free_mem, mload(0x40)), 32))
mstore(0x40, add(free_mem, 64))
}

return lInitCode;
Expand Down
2 changes: 1 addition & 1 deletion src/ReservoirDeployer.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ contract ReservoirDeployer {
uint256 public step = 0;

// Bytecode hashes.
bytes32 public constant FACTORY_HASH = bytes32(0xda6f297f6ce8cf60c5dcf260b6c4178f82c853ae2f16446c064cf6f4e2e4b1ca);
bytes32 public constant FACTORY_HASH = bytes32(0xaafa0bd4694a22c24f8ef26cfd45864f11be2102e269ee6a831d2c659b30f08f);
bytes32 public constant CONSTANT_PRODUCT_HASH =
bytes32(0xde6d8452b97c785350ce9f82090dfb5962933aa5467827310798925c8725013b);
bytes32 public constant STABLE_HASH = bytes32(0x1289a8879d4d1992d308d2a4d8457a5ca8d57e07effe092f2a1f21b0b9b10619);
Expand Down
68 changes: 68 additions & 0 deletions test/helpers/BytesLib.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// SPDX-License-Identifier: UNLICENSED
// taken from https://stackoverflow.com/questions/74443594/how-to-slice-bytes-memory-in-solidity
pragma solidity >=0.8.0;

library BytesLib {
function slice(bytes memory _bytes, uint256 _start, uint256 _length) internal pure returns (bytes memory) {
require(_length + 31 >= _length, "slice_overflow");
require(_bytes.length >= _start + _length, "slice_outOfBounds");

bytes memory tempBytes;

// Check length is 0. `iszero` return 1 for `true` and 0 for `false`.
assembly {
switch iszero(_length)
case 0 {
// Get a location of some free memory and store it in tempBytes as
// Solidity does for memory variables.
tempBytes := mload(0x40)

// Calculate length mod 32 to handle slices that are not a multiple of 32 in size.
let lengthmod := and(_length, 31)

// tempBytes will have the following format in memory: <length><data>
// When copying data we will offset the start forward to avoid allocating additional memory
// Therefore part of the length area will be written, but this will be overwritten later anyways.
// In case no offset is require, the start is set to the data region (0x20 from the tempBytes)
// mc will be used to keep track where to copy the data to.
let mc := add(add(tempBytes, lengthmod), mul(0x20, iszero(lengthmod)))
let end := add(mc, _length)

for {
// Same logic as for mc is applied and additionally the start offset specified for the method is added
let cc := add(add(add(_bytes, lengthmod), mul(0x20, iszero(lengthmod))), _start)
} lt(mc, end) {
// increase `mc` and `cc` to read the next word from memory
mc := add(mc, 0x20)
cc := add(cc, 0x20)
} {
// Copy the data from source (cc location) to the slice data (mc location)
mstore(mc, mload(cc))
}

// Store the length of the slice. This will overwrite any partial data that
// was copied when having slices that are not a multiple of 32.
mstore(tempBytes, _length)

// update free-memory pointer
// allocating the array padded to 32 bytes like the compiler does now
// To set the used memory as a multiple of 32, add 31 to the actual memory usage (mc)
// and remove the modulo 32 (the `and` with `not(31)`)
mstore(0x40, and(add(mc, 31), not(31)))
}
// if we want a zero-length slice let's just return a zero-length array
default {
tempBytes := mload(0x40)
// zero out the 32 bytes slice we are about to return
// we need to do it because Solidity does not garbage collect
mstore(tempBytes, 0)

// update free-memory pointer
// tempBytes uses 32 bytes in memory (even when empty) for the length.
mstore(0x40, add(tempBytes, 0x20))
}
}

return tempBytes;
}
}
38 changes: 38 additions & 0 deletions test/unit/GenericFactory.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,14 @@ import "test/__fixtures/BaseTest.sol";

import "test/__fixtures/MintableERC20.sol";

import { BytesLib } from "test/helpers/BytesLib.sol";
import { ConstantProductPair } from "src/curve/constant-product/ConstantProductPair.sol";
import { StablePair } from "src/curve/stable/StablePair.sol";
import { GenericFactory } from "src/GenericFactory.sol";

contract GenericFactoryTest is BaseTest {
using BytesLib for bytes;

function testCreatePair_AllCurves(uint256 aCurveId) public {
// assume
uint256 lCurveId = bound(aCurveId, 0, 1);
Expand Down Expand Up @@ -65,6 +68,28 @@ contract GenericFactoryTest is BaseTest {
_createPair(address(_tokenA), address(_tokenB), lCurveId);
}

function testCreatePair_Create2AddressCorrect() external {
// arrange
bytes32[] memory lCurves = _factory.curves();
address lPair1 = _factory.createPair(IERC20(address(_tokenC)), IERC20(address(_tokenD)), 0);
address lPair2 = _factory.createPair(IERC20(address(_tokenC)), IERC20(address(_tokenD)), 1);

bytes memory lInitBytecode1 = _tokenC < _tokenD
? _factory.getBytecode(lCurves[0], IERC20(address(_tokenC)), IERC20(address(_tokenD)))
: _factory.getBytecode(lCurves[0], IERC20(address(_tokenD)), IERC20(address(_tokenC)));
bytes memory lInitBytecode2 = _tokenC < _tokenD
? _factory.getBytecode(lCurves[1], IERC20(address(_tokenC)), IERC20(address(_tokenD)))
: _factory.getBytecode(lCurves[1], IERC20(address(_tokenD)), IERC20(address(_tokenC)));

// act
address lExpectedAddress1 = computeCreate2Address(bytes32(0), keccak256(lInitBytecode1), address(_factory));
address lExpectedAddress2 = computeCreate2Address(bytes32(0), keccak256(lInitBytecode2), address(_factory));

// assert
assertEq(lPair1, lExpectedAddress1);
assertEq(lPair2, lExpectedAddress2);
}

function testAllPairs() public {
// arrange
address lPair3 = _factory.createPair(IERC20(address(_tokenA)), IERC20(address(_tokenC)), 0);
Expand Down Expand Up @@ -107,4 +132,17 @@ contract GenericFactoryTest is BaseTest {
assertEq(_factory.getPair(IERC20(address(_tokenA)), IERC20(address(_tokenB)), 0), address(_constantProductPair));
assertEq(_factory.getPair(IERC20(address(_tokenB)), IERC20(address(_tokenA)), 0), address(_constantProductPair));
}

function testGetBytecode_CorrectConstructorData() external {
// arrange
bytes32[] memory lCurves = _factory.curves();

// act
bytes memory lBytecodeCP = _factory.getBytecode(lCurves[0], IERC20(address(_tokenA)), IERC20(address(_tokenB)));
bytes memory lBytecodeSP = _factory.getBytecode(lCurves[1], IERC20(address(_tokenC)), IERC20(address(_tokenB)));

// assert - the last bytes of the initCode should be the address of the second token, nothing more than that
assertEq0(lBytecodeCP.slice(lBytecodeCP.length - 32, 32), abi.encode(_tokenB));
assertEq0(lBytecodeSP.slice(lBytecodeSP.length - 32, 32), abi.encode(_tokenB));
}
}

0 comments on commit 8885123

Please sign in to comment.