Replace .transfer() with .call.value() and add destination to seizeBond
#189
Conversation
solidity/contracts/KeepBonding.sol
Outdated
| address operator, | ||
| uint256 referenceID, | ||
| uint256 amount, | ||
| address destination |
There was a problem hiding this comment.
address payable destination ?
| pragma solidity ^0.5.4; | ||
|
|
||
| /// @title Ether Transfer Receiver. | ||
| /// @dev This contract is for testing purposes only. |
There was a problem hiding this comment.
not strong on this one, but if this is for testing only, what do you think to make it clear in the name? Maybe by adding suffix Stub or Test?
solidity/test/ECDSAKeepTest.js
Outdated
| @@ -319,6 +323,35 @@ contract('ECDSAKeep', (accounts) => { | |||
| 'dividend value must be non-zero' | |||
| ) | |||
| }) | |||
|
|
|||
| it('doesn\'t revert in case of transfer failure', async () => { | |||
There was a problem hiding this comment.
maybe just does not to avoid adding special characters?
| // transfer failure, hence we don't validate it's result. | ||
| // TODO: What should we do with the dividend which was not transferred | ||
| // successfully? | ||
| members[i].call.value(dividend)(""); |
There was a problem hiding this comment.
This is a good change, but not because of the revert. send returns a boolean, but does not revert. you're probably thinking of transfer.
This is a good change though since op_code values have changed, and the default gas forwarding value of these functions (send, transfer) is no longer a safety feature. Need care handling .call() because it forwards all available gas, meaning we need to secure against reentrancy.
There was a problem hiding this comment.
on second thought, since call forwards all value, won't revert on a signer's callback be enough to consume all of the remaining gas, causing the entire tx to revert?
There was a problem hiding this comment.
on second thought, since call forwards all value, won't revert on a signer's callback be enough to consume all of the remaining gas, causing the entire tx to revert?
Does this test cover what you're asking?
https://github.com/keep-network/keep-tecdsa/blob/4db45638f4604dd358d70fe4fb1e3f537cc4e0a5/solidity/test/ECDSAKeepTest.js#L327-L354
There was a problem hiding this comment.
Yeah, that should be fine. I was just being paranoid. I also confirmed the behavior independently. It does as you would expect it to, but we still need to be extra careful with reentrancy. When ACL?
solidity/contracts/KeepBonding.sol
Outdated
| holder.transfer(amount); | ||
|
|
||
| (bool success, ) = destination.call.value(amount)(""); | ||
| require(success, "Transfer failed"); |
There was a problem hiding this comment.
Just a thought, if we've reached this state it is reasonable to assume that the caller is qualified to call this function.
Are we sure we want to revert here? implications could potentially be that a deposit flow breaks. Not sure myself need to think about it a little bit
There was a problem hiding this comment.
You may be right. We expect the caller to provide a valid destination address and we check the amount before. Let me know what you think after making sure 😄
| for (uint16 i = 0; i < memberCount; i++) { | ||
| // We don't want to revert the whole execution in case of single | ||
| // transfer failure, hence we don't validate it's result. | ||
| // TODO: What should we do with the dividend which was not transferred |
There was a problem hiding this comment.
my first thought was to not do anything for now, but we could split it among the other members. with a catch, in case all fail.
| pragma solidity ^0.5.4; | ||
|
|
||
| /// @title Ether Transfer Receiver. | ||
| /// @dev This contract is for testing purposes only. |
|
Resolve conflicts and we can merge |
The contract will be used in tests to simulate rejections of transfer sent to it. It will reject a transfer when value sent will be equal to `invalidValue` specified in the contract.
Replaced .transfer function for value forwarding with .call. This solution is [recommended by ConsenSys]: > #### Avoid `transfer()` and `send()`. > > `.transfer()` and `.send()` forward exactly 2,300 gas to the recipient. The goal of this hardcoded gas stipend was to prevent [reentrancy vulnerabilities](./known_attacks#reentrancy), but this only makes sense under the assumption that gas costs are constant. Recently [EIP 1283](https://eips.ethereum.org/EIPS/eip-1283) (backed out of the Constantinople hard fork at the last minute) and [EIP 1884](https://eips.ethereum.org/EIPS/eip-1884) (expected to arrive in the Istanbul hard fork) have shown this assumption to be invalid. > > To avoid things breaking when gas costs change in the future, it's best to use `.call.value(amount)("")` instead. Note that this does nothing to mitigate reentrancy attacks, so other precautions must be taken. [recommended by ConsenSys]: https://github.com/smarx/smart-contract-best-practices/blob/573013a54cfe1933e0189286bd2e74f99d96a319/docs/recommendations.md#avoid-transfer-and-send
Caller of the function (who is expected to be the holder of the bond) will have a possibility to transfer the bond to a specific destination. It not necesarrily need to be transfered to the holder. This will be used by ECDSA keep to transfer bonds to the owner of the keep.
nkuba
force-pushed
the
transfer-call
branch
2 times, most recently
from
eaed7a8 to
324a381
Compare
This PR contains two main changes:
Replace
.transfer()with.call.value()Replaced
.transferfunction for value forwarding with.call. This solution is recommended by ConsenSys:Add
destinationparameter toseizeBondfunctionPreviously we seized the bond by sending it to the bond's holder. With this change, we will allow the holder to specify a destination address where the funds should be transferred to. This will be used by ECDSA Keep which will be a holder of the bond to transfer the bond to the owner of the keep.
Refs: #171
Refs: #173