Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix cancel vulnerability #83

Merged
merged 2 commits into from
Sep 15, 2017
Merged

Fix cancel vulnerability #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d31ef7
Fix vulnerability
makoto Sep 15, 2017
963e06d
Change to check against sender’s address
makoto Sep 15, 2017
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
23 changes: 7 additions & 16 deletions contracts/Conference.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,20 +80,6 @@ contract Conference is Destructible {
}
}

modifier onlyPayable {
Participant participant = participants[msg.sender];
if (cancelled || (payoutAmount > 0 && participant.attended)){
_;
}
}

modifier notPaid {
Participant participant = participants[msg.sender];
if (participant.paid == false){
_;
}
}

modifier ifConfirmed(bytes32 _code){
require(confirmationRepository.claim(_code, msg.sender));
_;
Expand Down Expand Up @@ -167,10 +153,15 @@ contract Conference is Destructible {
AttendEvent(msg.sender);
}

function withdraw() public onlyPayable notPaid {
function withdraw() public{
require(payoutAmount > 0);
Participant participant = participants[msg.sender];
require(participant.addr == msg.sender);
require(cancelled || participant.attended);
require(participant.paid == false);

participant.paid = true;
assert(msg.sender.send(payoutAmount));
assert(participant.addr.send(payoutAmount));
WithdrawEvent(msg.sender, payoutAmount);
}

Expand Down
6 changes: 3 additions & 3 deletions log/stress_0002.log
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
type gasUsed gasPrice 1ETH*USD gasUsed*gasPrice(Ether) gasUsed*gasPrice(USD)
create 1673052 2 303 0.003346104 1.013869512
register 117847 2 303 0.000235694 0.071415282
create 1670512 2 303 0.003341024 1.012330272
register 117845 2 303 0.00023569 0.07141407
attend 51220 2 303 0.00010244 0.031039320000000002
payback 83265 2 303 0.00016653 0.05045859
withdraw 35648 2 303 0.000071296 0.021602688000000002
withdraw 35560 2 303 0.00007112 0.02154936
6 changes: 3 additions & 3 deletions log/stress_0020.log
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
type gasUsed gasPrice 1ETH*USD gasUsed*gasPrice(Ether) gasUsed*gasPrice(USD)
create 1673052 2 303 0.003346104 1.013869512
register 117847 2 303 0.000235694 0.071415282
create 1670512 2 303 0.003341024 1.012330272
register 117845 2 303 0.00023569 0.07141407
attend 51220 2 303 0.00010244 0.031039320000000002
payback 83265 2 303 0.00016653 0.05045859
withdraw 35648 2 303 0.000071296 0.021602688000000002
withdraw 35560 2 303 0.00007112 0.02154936
6 changes: 3 additions & 3 deletions log/stress_0100.log
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
type gasUsed gasPrice 1ETH*USD gasUsed*gasPrice(Ether) gasUsed*gasPrice(USD)
create 1673052 2 303 0.003346104 1.013869512
register 117847 2 303 0.000235694 0.071415282
create 1670512 2 303 0.003341024 1.012330272
register 117845 2 303 0.00023569 0.07141407
attend 51220 2 303 0.00010244 0.031039320000000002
payback 83265 2 303 0.00016653 0.05045859
withdraw 35648 2 303 0.000071296 0.021602688000000002
withdraw 35560 2 303 0.00007112 0.02154936
6 changes: 3 additions & 3 deletions log/stress_0200.log
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
type gasUsed gasPrice 1ETH*USD gasUsed*gasPrice(Ether) gasUsed*gasPrice(USD)
create 1673052 2 303 0.003346104 1.013869512
register 117847 2 303 0.000235694 0.071415282
create 1670512 2 303 0.003341024 1.012330272
register 117845 2 303 0.00023569 0.07141407
attend 51220 2 303 0.00010244 0.031039320000000002
payback 83265 2 303 0.00016653 0.05045859
withdraw 35648 2 303 0.000071296 0.021602688000000002
withdraw 35560 2 303 0.00007112 0.02154936
28 changes: 21 additions & 7 deletions test/conference.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,14 +182,14 @@ contract('Conference', function(accounts) {

it('cannot withdraw if non owner calls', async function(){
await conference.payback({from:non_owner}).catch(function(){});
await conference.withdraw({from:attended});
await conference.withdraw({from:attended}).catch(function(){});
// money is still left on contract
assert.strictEqual(web3.eth.getBalance(conference.address).toNumber(), deposit * 2);
})

it('cannot withdraw if you did not attend', async function(){
await conference.payback({from:owner});
await conference.withdraw({from:notAttended});
await conference.withdraw({from:notAttended}).catch(function(){});
// money is still left on contract
assert.strictEqual(web3.eth.getBalance(conference.address).toNumber(), deposit * 2);
})
Expand Down Expand Up @@ -238,7 +238,7 @@ contract('Conference', function(accounts) {

it('cannot cancel if non owner calls', async function(){
await conference.cancel({from:non_owner}).catch(function(){});
await conference.withdraw({from:attended});
await conference.withdraw({from:attended}).catch(function(){});
// money is still left on contract
assert.strictEqual(web3.eth.getBalance(conference.address).toNumber(), deposit * 2);
})
Expand Down Expand Up @@ -277,24 +277,38 @@ contract('Conference', function(accounts) {
it('cannot be canceled if the event is already ended', async function(){
await conference.payback();
await conference.cancel().catch(function(){});
await conference.withdraw({from:notAttended});
assert.strictEqual(web3.eth.getBalance(conference.address).toNumber(), deposit * 2);
await conference.withdraw({from:notAttended}).catch(function(){});
assert.strictEqual(web3.eth.getBalance(conference.address).toNumber(), deposit * 2);
await conference.withdraw({from:attended});
assert.strictEqual(web3.eth.getBalance(conference.address).toNumber(), 0);
assert.equal(await conference.ended.call(), true)
})
})

describe('on withdraw', function(){
it('cannot withdraw twice', async function(){
let registered = accounts[1];
let registered = accounts[1];
let notRegistered = accounts[2];

beforeEach(async function(){
await conference.register(twitterHandle, {from:owner, value:deposit});
await conference.register(twitterHandle, {from:registered, value:deposit});
assert.strictEqual( web3.eth.getBalance(conference.address).toNumber(), deposit * 2);
})

it('cannot withdraw twice', async function(){
await conference.cancel({from:owner});
await conference.withdraw({from:registered});
await conference.withdraw({from:registered});
await conference.withdraw({from:registered}).catch(function(){});
// only 1 ether is taken out
assert.strictEqual(web3.eth.getBalance(conference.address).toNumber(), deposit);
})

it('cannot withdraw if you did not register', async function(){
await conference.cancel({from:owner});
await conference.withdraw({from:notRegistered}).catch(function(){});
assert.strictEqual(web3.eth.getBalance(conference.address).toNumber(), deposit * 2);
})
})

describe('on clear', function(){
Expand Down