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 7 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
29 changes: 29 additions & 0 deletions contracts/mocks/Invalid.sol
Original file line number Diff line number Diff line change
@@ -17,3 +17,32 @@ 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";
}
}
38 changes: 38 additions & 0 deletions src/models/local/LocalBaseController.js
Original file line number Diff line number Diff line change
@@ -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. Avoid using low-level function 'delegatecall'. This is potentially a security risk. Please review and consider removing this call.`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change:
Avoid using low-level function 'delegatecall'. This is potentially a security risk.
To the following:
This is potentially a security risk, as the logic contract could be destructed by issuing a delegatecall to another contract with a selfdestruct instruction.

WDYT?

}
this.packageFile.addContract(contractAlias, contractName)
}

@@ -92,6 +102,34 @@ 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 ast = fs.parseJson(contractDataPath).ast
if (this.hasKeyValue(ast, "typeIdentifier", typeIdentifier)) return true
for (const node of ast.nodes) {
for (const baseContract of node.baseContracts || []) {
if (this.hasTypeIdentifier(Contracts.getLocalPath(baseContract.baseName.name), typeIdentifier)) 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) {
24 changes: 23 additions & 1 deletion test/scripts/add.test.js
Original file line number Diff line number Diff line change
@@ -92,10 +92,32 @@ 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: '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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test for WithParentWithDelegateCall

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

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