From cbed3ccef6af5c8726405b08bc40d2a4fbe1f130 Mon Sep 17 00:00:00 2001 From: Hardly Difficult Date: Thu, 9 Aug 2018 08:16:59 -0700 Subject: [PATCH] Warn if contract has a selfdestruct call (#347) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Warn if contract has a selfdestruct call Fix for #320 We are scanning the AST as suggested by @nventuro * Fixed test fails. I confirmed tests are passing locally this time ;) * Minor style change for consistency. * @spalladino Correct, the original approach did not work. I had misunderstood the structure of the AST. I made the changes you suggested and added a Test. Please let me know if this looks okay. I'm unsure how to address ancestor concern. I wrote a test for this, and commented it out because it fails ATM. If you have a suggestion on how to proceed, I'm happy to take a crack at it. * Addressing feedback I made changes addressing the code review feedback from @spalladino. For the parent implementations, I'm not sure how to go from node ID to the AST. I coded an alternative which is similar. Per the unit test it seems to work. Thoughts on this? * Added a delegatecall warning. Including a unit test for that warning. RE feedback: Changed the for loop style to for-of as suggested. I considered adding a warning for callcode as well, but there is already a message "Warning: "callcode" has been deprecated in favour of "delegatecall"." * Addressing feedback. Updating per the latest feedback. Note that I expect the test will still fail. I'm having trouble locally... When running the test suite, it simply skips the failing tests without message. ".... parseInit √ should not init √ should init with default when init is set √ should init when args is set √ should init with specific function √ should init with specific function and args Contract: add-all script E:\zos-cli2>" And when I try to run the test explicitly it also just terminates without messaging why.. "E:\zos-cli2>C:\Users\lopop\AppData\Roaming\npm\truffle test .\test\scripts\add-all.test.js Using network 'test'. Contract: add-all script E:\zos-cli2>" I tried one-off local tests (for all 4 test cases) and it seems to work fine. "E:\zostest>node ..\zos-cli2\lib\bin\zos-cli add SelfDestruct Compiling contracts Compiling .\contracts\Child.sol... Compiling .\contracts\Nested2.sol... Compiling .\contracts\SelfDestruct.sol... Writing artifacts to .\build\contracts Adding SelfDestruct Contract SelfDestruct (or its parent class) has a selfdestruct call. This is potentially a security risk. Please review and consider removing this call. Successfully written zos.json" I can't repro the fail. I'm unsure how to proceed at the moment. I'm going to set this down and try again later, let me know if you have any tips. thanks. * Fixed test fails. And addressed the feedback from @spalladino --- contracts/mocks/Invalid.sol | 38 ++++++++++++++++++ src/models/local/LocalBaseController.js | 39 +++++++++++++++++++ .../package-with-invalid-contracts.zos.json | 6 ++- test/scripts/add.test.js | 31 ++++++++++++++- 4 files changed, 112 insertions(+), 2 deletions(-) diff --git a/contracts/mocks/Invalid.sol b/contracts/mocks/Invalid.sol index 6012e6ea..b70f660e 100644 --- a/contracts/mocks/Invalid.sol +++ b/contracts/mocks/Invalid.sol @@ -17,3 +17,41 @@ contract WithFailingConstructor { assert(false); } } + +contract WithSelfDestruct { + uint256 public value; + + constructor() public { + if (true) + selfdestruct(msg.sender); + } + + function say() public pure returns (string) { + return "WithSelfDestruct"; + } +} + +contract WithParentWithSelfDestruct is WithSelfDestruct { + function say() public pure returns (string) { + return "WithParentWithSelfDestruct"; + } +} + +contract WithDelegateCall { + constructor(address _e) public { + require(_e.delegatecall(bytes4(keccak256("kill()")))); + } + + function say() public pure returns (string) { + return "WithDelegateCall"; + } +} + +contract WithParentWithDelegateCall is WithDelegateCall { + constructor(address _e) public WithDelegateCall(_e) { + } + + function say() public pure returns (string) { + return "WithParentWithDelegateCall"; + } +} \ No newline at end of file diff --git a/src/models/local/LocalBaseController.js b/src/models/local/LocalBaseController.js index 012788d6..61565f51 100644 --- a/src/models/local/LocalBaseController.js +++ b/src/models/local/LocalBaseController.js @@ -45,6 +45,16 @@ export default class LocalBaseController { if (this.hasConstructor(Contracts.getLocalPath(contractName))) { log.error(`Contract ${contractName} has an explicit constructor. Move it to an initializer function to use it with ZeppelinOS.`) } + // Log a warning anytime `selfdestruct` is found. This is a potential security risk, + // but not an error/throw as it may be a desired feature + if (this.hasSelfDestruct(Contracts.getLocalPath(contractName))) { + log.warn(`Contract ${contractName} (or its parent class) has a selfdestruct call. This is potentially a security risk. Please review and consider removing this call.`) + } + // Log a warning anytime `delegatecall` is found. This is a potential security risk, + // but not an error/throw as it may be a desired feature + if (this.hasDelegateCall(Contracts.getLocalPath(contractName))) { + log.warn(`Contract ${contractName} (or its parent class) has a delegatecall call. This is potentially a security risk, as the logic contract could be destructed by issuing a delegatecall to another contract with a selfdestruct instruction. Please review and consider removing this call.`) + } this.packageFile.addContract(contractAlias, contractName) } @@ -92,6 +102,35 @@ export default class LocalBaseController { return !!abi.find(fn => fn.type === "constructor"); } + hasSelfDestruct(contractDataPath) { + return this.hasTypeIdentifier(contractDataPath, "t_function_selfdestruct_nonpayable$_t_address_$returns$__$") + } + + hasDelegateCall(contractDataPath) { + return this.hasTypeIdentifier(contractDataPath, "t_function_baredelegatecall_nonpayable$__$returns$_t_bool_$") + } + + hasTypeIdentifier(contractDataPath, typeIdentifier) { + if (!fs.exists(contractDataPath)) return false + const contractJson = fs.parseJson(contractDataPath) + for (const node of contractJson.ast.nodes.filter((n) => n.name === contractJson.contractName)) { + if (this.hasKeyValue(node, "typeIdentifier", typeIdentifier)) return true + for (const baseContract of node.baseContracts || []) { + if (this.hasTypeIdentifier(Contracts.getLocalPath(baseContract.baseName.name), typeIdentifier)) return true + } + } + return false + } + + hasKeyValue(data, key, value) { + if (!data) return false + if (data[key] === value) return true + for (const childKey in data) { + if (typeof(data[childKey]) === 'object' && this.hasKeyValue(data[childKey], key, value)) return true + } + return false + } + getContractClass(contractAlias) { const contractName = this.packageFile.contract(contractAlias); if (contractName) { diff --git a/test/mocks/packages/package-with-invalid-contracts.zos.json b/test/mocks/packages/package-with-invalid-contracts.zos.json index 4b6d8fb3..5c908cd3 100644 --- a/test/mocks/packages/package-with-invalid-contracts.zos.json +++ b/test/mocks/packages/package-with-invalid-contracts.zos.json @@ -3,6 +3,10 @@ "version": "1.1.0", "contracts": { "Impl": "ImplV1", - "WithFailingConstructor": "WithFailingConstructor" + "WithFailingConstructor": "WithFailingConstructor", + "WithSelfDestruct": "WithSelfDestruct", + "WithParentWithSelfDestruct": "WithParentWithSelfDestruct", + "WithDelegateCall": "WithDelegateCall", + "WithParentWithDelegateCall": "WithParentWithDelegateCall" } } diff --git a/test/scripts/add.test.js b/test/scripts/add.test.js index 9096b2c4..0ae048d2 100644 --- a/test/scripts/add.test.js +++ b/test/scripts/add.test.js @@ -92,10 +92,39 @@ contract('add script', function() { this.logs.errors[0].should.match(/constructor/i); }); - it('should not warn when adding a contract without a constructor', async function() { + it('should not warn when adding a contract without a constructor or a selfdestruct call', async function() { add({ contractsData, packageFile: this.packageFile }); this.logs.errors.should.have.lengthOf(0); + this.logs.warns.should.have.lengthOf(0); + }); + + it('should warn when adding a contract with a selfdestruct call', async function() { + add({ contractsData: [{ name: 'WithSelfDestruct', alias: 'WithSelfDestruct' }], packageFile: this.packageFile }); + + this.logs.warns.should.have.lengthOf(1); + this.logs.warns[0].should.match(/selfdestruct/i); + }); + + it('should warn when adding a contract whose parent has a selfdestruct call', async function() { + add({ contractsData: [{ name: 'WithParentWithSelfDestruct', alias: 'WithParentWithSelfDestruct' }], packageFile: this.packageFile }); + + this.logs.warns.should.have.lengthOf(1); + this.logs.warns[0].should.match(/selfdestruct/i); + }); + + it('should warn when adding a contract with a delegatecall call', async function() { + add({ contractsData: [{ name: 'WithDelegateCall', alias: 'WithDelegateCall' }], packageFile: this.packageFile }); + + this.logs.warns.should.have.lengthOf(1); + this.logs.warns[0].should.match(/delegatecall/i); + }); + + it('should warn when adding a contract whose parent has a delegatecall call', async function() { + add({ contractsData: [{ name: 'WithParentWithDelegateCall', alias: 'WithParentWithDelegateCall' }], packageFile: this.packageFile }); + + this.logs.warns.should.have.lengthOf(1); + this.logs.warns[0].should.match(/delegatecall/i); }); }); });