Deploying a delegate with incomplete bytecode

[code423n4]

Lines of code

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721DelegateDeployer.sol#L83 https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721DelegateDeployer.sol#L115

Vulnerability details

Vulnerability details

Description

Function deployDelegateFor in the JBTiered721DelegateDeployer deploys a copied bytecode from the reference smart contracts. That's implemented in the _clone function. Specifically, it is saved in the memory init code and concatenated with the copied bytecode.

  function _clone(address _targetAddress) internal returns (address _out) {
    assembly {
      // Get deployed/runtime code size
      let _codeSize := extcodesize(_targetAddress)

      // Get a bit of freemem to land the bytecode, not updated as we'll leave this scope right after create(..)
      let _freeMem := mload(0x40)

      // Shift the length to the length placeholder, in the constructor
      let _mask := mul(_codeSize, 0x100000000000000000000000000000000000000000000000000000000)

      // Insert the length in the correct sport (after the PUSH3 / 0x62)
      let _initCode := or(_mask, 0x62000000600081600d8239f3fe00000000000000000000000000000000000000)

      // Store the deployment bytecode
      mstore(_freeMem, _initCode)

      // Copy the bytecode (our initialise part is 13 bytes long)
      extcodecopy(_targetAddress, add(_freeMem, 13), 0, _codeSize)

      // Deploy the copied bytecode
      _out := create(0, _freeMem, _codeSize)
    }
  }

Concatenated creation code stored in the _freeMem and has 13 + _codeSize bytes length. However, the bytecode that is gonna be deployed has only _codeSize.

      _out := create(0, _freeMem, _codeSize)

So, the _copy method deploys not the copied bytecode, but the bytecode without the last 13 bytes.

Recommended Mitigation Steps

Deploy full bytecode, by

    _out := create(0, _freeMem, add(13, _codeSize))

[trust1995]

It is a correct observation, but submission does not state any actual impact caused by this. In fact, this removes the compiler metadata stored at the end of the contract, which means the contract may not be verified successfully on etherscan. For High risk, submission must attempt to demonstrate funds at risk, which is very far from the case.

[drgorillamd]

Really nice finding! Will have to study attack surface further, don't really see how this could create a risk for funds (fwiw, we're aiming to have verification by "Similar bytecode" on etherscan, as the canonical contract will be the verified ones - they do have the metadata at the end, the only remaining unknow is therefore if etherscan checks them or just the runtime bytecode)

[drgorillamd]

Really nice finding! Will have to study attack surface further, don't really see how this could create a risk for funds (fwiw, we're aiming to have verification by "Similar bytecode" on etherscan, as the canonical contract will be the verified ones - they do have the metadata at the end, the only remaining unknow is therefore if etherscan checks them or just the runtime bytecode)

So yeah, after trying, the only thing at stake here is lack of similar bytecode verification possibility - thanks:)

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-a