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

Warn about duplicated super constructor calls #3305

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
4 participants
@federicobond
Contributor

federicobond commented Dec 11, 2017

Mentioned in the audit report from Coinspect available here.

I think I saw an issue related to this, but I couldn't find it.

FIxes #3325

@LianaHus LianaHus added needs review and removed needs review labels Dec 11, 2017

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Dec 12, 2017

Member

I think this should be an error and not a warning given the value from the "first" (aka the inheritance definition) call is ignored.

contract A {
  uint kk = 42;
  function A(uint a) {
    kk = a;
  }
}

contract B is A(3) {
  function B() A(5) {
  }
}

generates

    /* "constructor.sol":71:119  contract B is A(3) {... */
  mstore(0x40, 0x60)
    /* "constructor.sol":25:27  42 */
  0x2a
    /* "constructor.sol":15:27  uint kk = 42 */
  0x0
  sstore
    /* "constructor.sol":94:117  function B() A(5) {... */
  jumpi(tag_1, iszero(callvalue))
  0x0
  dup1
  revert
tag_1:
    /* "constructor.sol":109:110  5 */
  0x5
    /* "constructor.sol":61:62  a */
  dup1
    /* "constructor.sol":56:58  kk */
  0x0
    /* "constructor.sol":56:62  kk = a */
  dup2
  swap1
  sstore
  pop
    /* "constructor.sol":31:67  function A(uint a) {... */
  pop
    /* "constructor.sol":71:119  contract B is A(3) {... */
  dataSize(sub_0)
  dup1
  dataOffset(sub_0)
  0x0
  codecopy
  0x0
  return
stop

sub_0: assembly {
        /* "constructor.sol":71:119  contract B is A(3) {... */
      mstore(0x40, 0x60)
      0x0
      dup1
      revert

    auxdata: 0xa165627a7a72305820bb344fe8288d7f50fc55f376d5b621f6ff582f7dae9c855821706d6f9bb8346d0029
}

Also it should do this check for calling the constructor twice without an argument too.

And please include a changelog entry.

Member

axic commented Dec 12, 2017

I think this should be an error and not a warning given the value from the "first" (aka the inheritance definition) call is ignored.

contract A {
  uint kk = 42;
  function A(uint a) {
    kk = a;
  }
}

contract B is A(3) {
  function B() A(5) {
  }
}

generates

    /* "constructor.sol":71:119  contract B is A(3) {... */
  mstore(0x40, 0x60)
    /* "constructor.sol":25:27  42 */
  0x2a
    /* "constructor.sol":15:27  uint kk = 42 */
  0x0
  sstore
    /* "constructor.sol":94:117  function B() A(5) {... */
  jumpi(tag_1, iszero(callvalue))
  0x0
  dup1
  revert
tag_1:
    /* "constructor.sol":109:110  5 */
  0x5
    /* "constructor.sol":61:62  a */
  dup1
    /* "constructor.sol":56:58  kk */
  0x0
    /* "constructor.sol":56:62  kk = a */
  dup2
  swap1
  sstore
  pop
    /* "constructor.sol":31:67  function A(uint a) {... */
  pop
    /* "constructor.sol":71:119  contract B is A(3) {... */
  dataSize(sub_0)
  dup1
  dataOffset(sub_0)
  0x0
  codecopy
  0x0
  return
stop

sub_0: assembly {
        /* "constructor.sol":71:119  contract B is A(3) {... */
      mstore(0x40, 0x60)
      0x0
      dup1
      revert

    auxdata: 0xa165627a7a72305820bb344fe8288d7f50fc55f376d5b621f6ff582f7dae9c855821706d6f9bb8346d0029
}

Also it should do this check for calling the constructor twice without an argument too.

And please include a changelog entry.

@federicobond

This comment has been minimized.

Show comment
Hide comment
@federicobond

federicobond Dec 12, 2017

Contributor

Good, I agree it should be an error. I think the case of "calling the constructor twice without an argument" is already covered by other errors, perhaps I'm missing something?

Contributor

federicobond commented Dec 12, 2017

Good, I agree it should be an error. I think the case of "calling the constructor twice without an argument" is already covered by other errors, perhaps I'm missing something?

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Dec 12, 2017

Member

This compiles without issues, don't think it is covered:

contract A {}
contract B is A() {
  function B() A() {
  }
}
Member

axic commented Dec 12, 2017

This compiles without issues, don't think it is covered:

contract A {}
contract B is A() {
  function B() A() {
  }
}

@axic axic added the nextrelease label Dec 12, 2017

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Dec 12, 2017

Member

Please add this test too:

contract B is A { function B() A() {} }
Member

axic commented Dec 12, 2017

Please add this test too:

contract B is A { function B() A() {} }
Show outdated Hide outdated Changelog.md
}
contract Derived2 is Base {
function Derived2(uint _y) Base(_y * _y) public {}

This comment has been minimized.

@axic

axic Dec 18, 2017

Member

Should we also warn if the constructor has a parameter but it is not explicitly initialised?

@axic

axic Dec 18, 2017

Member

Should we also warn if the constructor has a parameter but it is not explicitly initialised?

This comment has been minimized.

@chriseth

chriseth Dec 18, 2017

Contributor

This is a good example where this is useful, namely if the parameter depends on the constructor parameter of the derived contract. I hope we already have a warning if the parameters are specified nowhere...

@chriseth

chriseth Dec 18, 2017

Contributor

This is a good example where this is useful, namely if the parameter depends on the constructor parameter of the derived contract. I hope we already have a warning if the parameters are specified nowhere...

This comment has been minimized.

@axic

axic Jan 3, 2018

Member

I hope we already have a warning if the parameters are specified nowhere...

No, it just defaults to the type default.

@axic

axic Jan 3, 2018

Member

I hope we already have a warning if the parameters are specified nowhere...

No, it just defaults to the type default.

This comment has been minimized.

@federicobond

federicobond Jan 3, 2018

Contributor

It’s impossible to know whether the contract is deployable as it is or meant as a base contract, so if we enforce this, the contract that specifies a base contract must also provide an explicit, though overridable, argument for all base contract parameters.

@federicobond

federicobond Jan 3, 2018

Contributor

It’s impossible to know whether the contract is deployable as it is or meant as a base contract, so if we enforce this, the contract that specifies a base contract must also provide an explicit, though overridable, argument for all base contract parameters.

This comment has been minimized.

@axic

axic Jan 4, 2018

Member

One more reason to have an explicit abstract marker.

@axic

axic Jan 4, 2018

Member

One more reason to have an explicit abstract marker.

This comment has been minimized.

@axic

axic Jan 4, 2018

Member

Constructors could be chained through the abstract contracts explicitly, but that is a breaking change.

@axic

axic Jan 4, 2018

Member

Constructors could be chained through the abstract contracts explicitly, but that is a breaking change.

@federicobond

This comment has been minimized.

Show comment
Hide comment
@federicobond

federicobond Dec 24, 2017

Contributor

I have submitted a PR for the failing Zeppelin code.

Contributor

federicobond commented Dec 24, 2017

I have submitted a PR for the failing Zeppelin code.

@federicobond

This comment has been minimized.

Show comment
Hide comment
@federicobond

federicobond Jan 9, 2018

Contributor

@axic anything blocking this?

Contributor

federicobond commented Jan 9, 2018

@axic anything blocking this?

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Feb 5, 2018

Member

@federicobond honestly I don't remember the status of this.

Member

axic commented Feb 5, 2018

@federicobond honestly I don't remember the status of this.

@federicobond

This comment has been minimized.

Show comment
Hide comment
@federicobond

federicobond Feb 5, 2018

Contributor

@axic the implementation is finished but it's a breaking change (as the OpenZeppelin failure demonstrates). The change is enforced only within a contract, where it's obvious that the duplicated constructor was not intended or noticed, not across the whole inheritance graph, which would be more difficult.

Contributor

federicobond commented Feb 5, 2018

@axic the implementation is finished but it's a breaking change (as the OpenZeppelin failure demonstrates). The change is enforced only within a contract, where it's obvious that the duplicated constructor was not intended or noticed, not across the whole inheritance graph, which would be more difficult.

@axic axic removed the nextrelease label Feb 9, 2018

@chriseth chriseth added this to To do in 0.5.0 via automation Mar 6, 2018

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Mar 6, 2018

Contributor

This just requires a small change to work with openzeppelin: If the base constructor does not take arguments, it is fine to mention it twice.

Contributor

chriseth commented Mar 6, 2018

This just requires a small change to work with openzeppelin: If the base constructor does not take arguments, it is fine to mention it twice.

@ekpyron ekpyron referenced this pull request Apr 4, 2018

Merged

Warn constructor override #3821

0.5.0 automation moved this from To do to Done Apr 10, 2018

@axic axic added this to Done in Coinspect Audit Aug 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment