Skip to content
This repository has been archived by the owner on Jul 11, 2019. It is now read-only.

Warn if contract has a selfdestruct call #347

Merged
merged 9 commits into from
Aug 9, 2018
Merged
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions contracts/mocks/Invalid.sol
Original file line number Diff line number Diff line change
@@ -17,3 +17,19 @@ 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 ParentHasSelfDestruct is WithSelfDestruct {
}
26 changes: 26 additions & 0 deletions src/models/local/LocalBaseController.js
Original file line number Diff line number Diff line change
@@ -45,6 +45,11 @@ 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} has a selfdestruct call. This is potentially a security risk. Please review and consider removing this call.`)
}
this.packageFile.addContract(contractAlias, contractName)
}

@@ -92,6 +97,27 @@ export default class LocalBaseController {
return !!abi.find(fn => fn.type === "constructor");
}

hasSelfDestruct(contractDataPath) {
if (!fs.exists(contractDataPath)) return false
const ast = fs.parseJson(contractDataPath).ast
if (this.hasKeyValue(ast, "typeIdentifier", "t_function_selfdestruct_nonpayable$_t_address_$returns$__$")) return true
for (let i = 0; i < ast.nodes.length; i++) {
for (let j = 0; j < (ast.nodes.baseContracts || []).length; j++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change these for-loops to use either the for-of construct for forEach method, for better readability?

const contractName = ast.nodes.baseContracts.baseName.name
if (this.hasSelfDestruct(Contracts.getLocalPath(contractName))) return true
}
}
}

hasKeyValue(data, key, value) {
if (!data) return false
if (data[key] === value) return true

This comment was marked as resolved.

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) {
17 changes: 16 additions & 1 deletion test/scripts/add.test.js
Original file line number Diff line number Diff line change
@@ -92,10 +92,25 @@ 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 who\'s parent has a selfdestruct call', async function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

who' s => whose (sorry, grammar nazi here)

add({ contractsData: [{ name: 'ParentHasSelfDestruct', alias: 'ParentHasSelfDestruct' }], packageFile: this.packageFile });

this.logs.warns.should.have.lengthOf(1);
this.logs.warns[0].should.match(/selfdestruct/i);
});
});
});