Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider changing the _deployData architecture #114

Open
code423n4 opened this issue Sep 29, 2021 · 0 comments
Open

Consider changing the _deployData architecture #114

code423n4 opened this issue Sep 29, 2021 · 0 comments
Labels
bug Warden finding G (Gas Optimization) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Handle

hrkrshnn

Vulnerability details

Consider changing the _deployData architecture

Currently, the factory contracts implements deployment in the following
fashion:

  1. The argument to the external function has the type bytes memory.
  2. This gets decoded using abi.decode(deploydata, args)

Example

Consider directly specifying the parameters.

modified   contracts/pool/ConstantProductPoolFactory.sol
@@ -10,8 +10,7 @@ import "./PoolDeployer.sol";
 contract ConstantProductPoolFactory is PoolDeployer {
     constructor(address _masterDeployer) PoolDeployer(_masterDeployer) {}

-    function deployPool(bytes memory _deployData) external returns (address pool) {
-        (address tokenA, address tokenB, uint256 swapFee, bool twapSupport) = abi.decode(_deployData, (address, address, uint256, bool));
+    function deployPool(address tokenA, address tokenB, uint256 swapFree, bool twapSupport) external returns (address pool) {
         if (tokenA > tokenB) {
             (tokenA, tokenB) = (tokenB, tokenA);
             _deployData = abi.encode(tokenA, tokenB, swapFee, twapSupport);

Advantages

  1. Using _deployData instead of (address, address, uint256, uint256) needs larger calldata size. This is because the bytes
    also needs to store the length.
  2. This avoids going via memory. For the byte array example, the value
    is first copied from calldata to memory (note: solidity
    currently does this in an inefficient for loop, instead of using
    calldatacopy; so if you really want to keep the architecture,
    writing it in assembly using calldatacopy would be even more
    efficient.) Effectively, this saves the cost to store in memory, as
    well as memory expansion costs. Since the function call does a
    contract deployment, the savings from memory expansion costs can be
    reasonably high, assuming that the quadratic pricing is reached.

This pattern seems to be used in several other places. All of them can
be better optimized.

  1. https://github.com/sushiswap/trident/blob/9130b10efaf9c653d74dc7a65bde788ec4b354b5/contracts/pool/ConstantProductPool.sol#L124
code423n4 added a commit that referenced this issue Sep 29, 2021
@maxsam4 maxsam4 added sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons and removed sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue labels Oct 19, 2021
@maxsam4 maxsam4 closed this as completed Oct 19, 2021
@ninek9 ninek9 reopened this Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Warden finding G (Gas Optimization) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

3 participants