Skip to content

Commit

Permalink
Merge pull request #251 from err508/simplify_withdraw
Browse files Browse the repository at this point in the history 
Removes redundant partner argument from setTotalWithdraw.
  • Loading branch information
@loredanacirstea
loredanacirstea authored Aug 23, 2018
2 parents 24cb094 + 6bb090d commit 15d7328
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 179 deletions.
88 changes: 34 additions & 54 deletions raiden_contracts/contracts/TokenNetwork.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,78 +329,89 @@ contract TokenNetwork is Utils {
/// amount.
/// @param total_withdraw Total amount of tokens that are marked as
/// withdrawn from the channel during the channel lifecycle.
/// @param partner Channel partner address, needed to compute the total
/// channel deposit.
/// @param participant_signature Participant's signature on the withdraw
/// data.
/// @param partner_signature Partner's signature on the withdraw data.
function setTotalWithdraw(
uint256 channel_identifier,
address participant,
uint256 total_withdraw,
address partner,
bytes participant_signature,
bytes partner_signature
)
isOpen(channel_identifier)
external
{
require(channel_identifier == getChannelIdentifier(participant, partner));
uint256 total_deposit;
uint256 current_withdraw;
address partner;

require(total_withdraw > 0);

verifyWithdrawSignatures(
// Authenticate both channel partners via there signatures:
require(participant == recoverAddressFromWithdrawMessage(
channel_identifier,
participant,
total_withdraw,
participant_signature
));
partner = recoverAddressFromWithdrawMessage(
channel_identifier,
participant,
partner,
total_withdraw,
participant_signature,
partner_signature
);

uint256 total_deposit;
uint256 current_withdraw;
// Validate that authenticated partners and the channel identifier match
require(channel_identifier == getChannelIdentifier(participant, partner));

// Read channel state after validating the function input
Channel storage channel = channels[channel_identifier];
Participant storage participant_state = channel.participants[participant];
Participant storage partner_state = channel.participants[partner];

total_deposit = participant_state.deposit + partner_state.deposit;
// This should never happen, as we have an overflow check in setTotalDeposit
assert(total_deposit >= participant_state.deposit);
assert(total_deposit >= partner_state.deposit);

// Entire withdrawn amount must not be bigger than the current channel deposit
require(total_withdraw <= (total_deposit - partner_state.withdrawn_amount));
require((total_withdraw + partner_state.withdrawn_amount) <= total_deposit);
require(total_withdraw <= (total_withdraw + partner_state.withdrawn_amount));

// Using the total_withdraw (monotonically increasing) in the signed
// message ensures that we do not allow reply attack to happen, by
// message ensures that we do not allow replay attack to happen, by
// using the same withdraw proof twice.
// Next two lines enforce the monotonicity of total_withdraw and check for an underflow:
// (we use <= because current_withdraw == total_withdraw for the first withdraw)
current_withdraw = total_withdraw - participant_state.withdrawn_amount;
// The actual amount of tokens that will be transferred must be > 0
require(current_withdraw > 0);
// Underflow check; we use <= because current_withdraw == total_withdraw for the first
// withdraw
require(current_withdraw <= total_withdraw);

// The actual amount of tokens that will be transferred must be > 0 to disable the reuse of
// withdraw messages completely.
require(current_withdraw > 0);

// This should never fail at this point. Added check for security, because we directly set
// the participant_state.withdrawn_amount = total_withdraw,
// while we transfer `current_withdraw` tokens.
assert(participant_state.withdrawn_amount + current_withdraw == total_withdraw);

participant_state.withdrawn_amount = total_withdraw;

emit ChannelWithdraw(
channel_identifier,
participant,
participant_state.withdrawn_amount
total_withdraw
);

// Do the tokens transfer
// Do the state change and tokens transfer
participant_state.withdrawn_amount = total_withdraw;
require(token.transfer(participant, current_withdraw));

// This should never happen, as we have an overflow check in setTotalDeposit
assert(total_deposit >= participant_state.deposit);
assert(total_deposit >= partner_state.deposit);

// A withdraw should never happen if a participant already has a
// balance proof in storage
// balance proof in storage. This should never fail as we use isOpen.
assert(participant_state.nonce == 0);
assert(partner_state.nonce == 0);

}

/// @notice Close the channel defined by the two participant addresses. Only
Expand Down Expand Up @@ -1338,37 +1349,6 @@ contract TokenNetwork is Utils {
signature_address = ECVerify.ecverify(message_hash, signature);
}

function verifyWithdrawSignatures(
uint256 channel_identifier,
address participant,
address partner,
uint256 total_withdraw,
bytes participant_signature,
bytes partner_signature
)
view
internal
{
address recovered_participant_address;
address recovered_partner_address;

recovered_participant_address = recoverAddressFromWithdrawMessage(
channel_identifier,
participant,
total_withdraw,
participant_signature
);
recovered_partner_address = recoverAddressFromWithdrawMessage(
channel_identifier,
participant,
total_withdraw,
partner_signature
);
// Check recovered addresses from signatures
require(participant == recovered_participant_address);
require(partner == recovered_partner_address);
}

/// @dev Calculates the merkle root for the pending transfers data and
//calculates the amount / of tokens that can be unlocked because the secret
//was registered on-chain.
Expand Down
21 changes: 0 additions & 21 deletions raiden_contracts/contracts/test/TokenNetworkInternalsTest.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,27 +218,6 @@ contract TokenNetworkInternalsTest is TokenNetwork {
);
}

function verifyWithdrawSignaturesPublic(
uint256 channel_identifier,
address participant,
address partner,
uint256 total_withdraw,
bytes participant_signature,
bytes partner_signature
)
view
public
{
return verifyWithdrawSignatures(
channel_identifier,
participant,
partner,
total_withdraw,
participant_signature,
partner_signature
);
}

function getMerkleRootAndUnlockedAmountPublic(bytes merkle_tree_leaves)
view
public
Expand Down
1 change: 0 additions & 1 deletion raiden_contracts/tests/fixtures/channel.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ def get(channel_identifier, participant, withdraw_amount, partner, delegate=None
channel_identifier,
participant,
withdraw_amount,
partner,
signature_participant,
signature_partner
).transact({'from': delegate})
Expand Down
49 changes: 0 additions & 49 deletions raiden_contracts/tests/test_channel_withdraw.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ def test_withdraw_call(
channel_identifier,
0x0,
withdraw_A,
B,
signature_A_for_A,
signature_B_for_A,
).transact({'from': A})
Expand All @@ -43,25 +42,6 @@ def test_withdraw_call(
channel_identifier,
'',
withdraw_A,
B,
signature_A_for_A,
signature_B_for_A,
).transact({'from': A})
with pytest.raises(ValidationError):
token_network.functions.setTotalWithdraw(
channel_identifier,
A,
withdraw_A,
0x0,
signature_A_for_A,
signature_B_for_A,
).transact({'from': A})
with pytest.raises(ValidationError):
token_network.functions.setTotalWithdraw(
channel_identifier,
A,
withdraw_A,
'',
signature_A_for_A,
signature_B_for_A,
).transact({'from': A})
Expand All @@ -70,7 +50,6 @@ def test_withdraw_call(
channel_identifier,
A,
-1,
B,
signature_A_for_A,
signature_B_for_A,
).transact({'from': A})
Expand All @@ -79,7 +58,6 @@ def test_withdraw_call(
channel_identifier,
A,
MAX_UINT256 + 1,
B,
signature_A_for_A,
signature_B_for_A,
).transact({'from': A})
Expand All @@ -89,16 +67,6 @@ def test_withdraw_call(
channel_identifier,
EMPTY_ADDRESS,
withdraw_A,
B,
signature_A_for_A,
signature_B_for_A,
).transact({'from': A})
with pytest.raises(TransactionFailed):
token_network.functions.setTotalWithdraw(
channel_identifier,
A,
withdraw_A,
EMPTY_ADDRESS,
signature_A_for_A,
signature_B_for_A,
).transact({'from': A})
Expand All @@ -107,7 +75,6 @@ def test_withdraw_call(
channel_identifier,
A,
0,
B,
signature_A_for_A,
signature_B_for_A,
).transact({'from': A})
Expand All @@ -116,7 +83,6 @@ def test_withdraw_call(
channel_identifier,
A,
withdraw_A,
B,
fake_bytes(65),
signature_B_for_A,
).transact({'from': A})
Expand All @@ -125,7 +91,6 @@ def test_withdraw_call(
channel_identifier,
A,
withdraw_A,
B,
signature_A_for_A,
fake_bytes(65),
).transact({'from': A})
Expand All @@ -134,7 +99,6 @@ def test_withdraw_call(
channel_identifier,
A,
withdraw_A,
B,
signature_A_for_A,
signature_B_for_A,
).transact({'from': A})
Expand Down Expand Up @@ -245,7 +209,6 @@ def test_withdraw_wrong_signers(
channel_identifier,
A,
withdraw_A,
B,
signature_C_for_A,
signature_B_for_A,
).transact({'from': C})
Expand All @@ -254,7 +217,6 @@ def test_withdraw_wrong_signers(
channel_identifier,
A,
withdraw_A,
B,
signature_A_for_A,
signature_C_for_A,
).transact({'from': C})
Expand All @@ -263,7 +225,6 @@ def test_withdraw_wrong_signers(
channel_identifier,
A,
withdraw_A,
B,
signature_A_for_A,
signature_B_for_A,
).transact({'from': C})
Expand Down Expand Up @@ -314,7 +275,6 @@ def test_withdraw_wrong_signature_content(
channel_identifier,
A,
withdraw_A,
B,
signature_A_for_A_fake1,
signature_B_for_A,
).transact({'from': A})
Expand All @@ -323,7 +283,6 @@ def test_withdraw_wrong_signature_content(
channel_identifier,
A,
withdraw_A,
B,
signature_A_for_A,
signature_B_for_A_fake1,
).transact({'from': A})
Expand All @@ -332,7 +291,6 @@ def test_withdraw_wrong_signature_content(
channel_identifier,
A,
withdraw_A,
B,
signature_A_for_A_fake2,
signature_B_for_A,
).transact({'from': A})
Expand All @@ -341,7 +299,6 @@ def test_withdraw_wrong_signature_content(
channel_identifier,
A,
withdraw_A,
B,
signature_A_for_A,
signature_B_for_A_fake2,
).transact({'from': A})
Expand All @@ -350,7 +307,6 @@ def test_withdraw_wrong_signature_content(
channel_identifier,
A,
withdraw_A,
B,
signature_A_for_A_fake3,
signature_B_for_A,
).transact({'from': A})
Expand All @@ -359,7 +315,6 @@ def test_withdraw_wrong_signature_content(
channel_identifier,
A,
withdraw_A,
B,
signature_A_for_A,
signature_B_for_A_fake3,
).transact({'from': A})
Expand All @@ -368,7 +323,6 @@ def test_withdraw_wrong_signature_content(
channel_identifier,
A,
withdraw_A,
B,
signature_A_for_A,
signature_B_for_A,
).transact({'from': A})
Expand Down Expand Up @@ -486,7 +440,6 @@ def test_withdraw_replay_reopened_channel(
channel_identifier1,
A,
withdraw_A,
B,
signature_A_for_A,
signature_B_for_A,
).transact({'from': A})
Expand Down Expand Up @@ -522,7 +475,6 @@ def test_withdraw_replay_reopened_channel(
channel_identifier2,
A,
withdraw_A,
B,
signature_A_for_A,
signature_B_for_A,
).transact({'from': A})
Expand All @@ -538,7 +490,6 @@ def test_withdraw_replay_reopened_channel(
channel_identifier2,
A,
withdraw_A,
B,
signature_A_for_A2,
signature_B_for_A2,
).transact({'from': A})
Expand Down
Loading

0 comments on commit 15d7328

Please sign in to comment.