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); }); }); });