Conversation
Seize Signer bonds, send new keep fee estimate to the TDT owner, return the rest.
send() is considered unsafe due to not reverting. This functinality is useful here.
tbtcSystemStub functinality is needed in createNewDeposit.
…into sig-err-refund
Use keep setup fee at time of Deposit creation to refund the funder inc ase of signer setup failure. This does not use the Value sent to createNewDeposit() since this is not bounded anywhere.
@@ -65,6 +65,7 @@ library DepositFunding { | |||
uint256 _bondRequirementSatoshi = _lotSizeSatoshis.mul(_system.getInitialCollateralizedPercent()).div(100); | |||
uint256 _bondRequirementWei = _d.fetchBitcoinPrice().mul(_bondRequirementSatoshi); | |||
|
|||
_d.keepSetupFee = _system.createNewDepositFeeEstimate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it should be = msg.value
, no? That's what we actually forward to the keep, should be what we capture here also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought so initially, but msg.value
is unbounded and that's a potential hole. It's not very realistic so if you think it's not worth covering, we record msg.value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see… So the depositor could send a greater msg.value
than the bond amount, and then when we try and refund them we have insufficient seized bond to do it, and things go sideways. We also give the depositor the ability to take more from signers than they are really due in the case where signer setup fails.
I think the thing we need to do here is use the lesser of msg.sender
and _system.createNewDepositFeeEstimate
. This protects against situations where the depositor sends more or less than the deposit fee estimate and then gets repaid. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if createNewDepositFeeEstimate
is more than msg.value
does the tx still go through? I'm not sure how this works on the Keep side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it goes through + surplus is captured in the system to subsidize future entries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My q was with msg.value < createNewDepositFeeEstimate case, Don't know if this fails on Keep side.
msg.value < openKeepFeeEstimate()
will revert on openKeep
call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's chat about this tomorrow, I think. It feels like just a minor griefing vector
After you discuss it please share your findings here, my thought was also that we should use msg.value
here as @Shadowfiend mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's aim to capture msg.value
and use that for refunding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the rationale for msg.value
over openKeepFeeEstimate
?
It protects users from receiving less if they overfunded, but I don't think that should be at the signers' expense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't see a great way out of this.
If we refund based on the estimate, and the estimate is less than the provided value, the funder loses the spread without doing anything wrong. If everyone is behaving correctly and honestly, the value loss is small. If someone is misbehaving, the value loss is still bounded and small, since the keep estimates are part of the system rather than controlled by any of the parties in this particular interaction.
If we refund based on the value, the funder gets fully refunded, but may take more than they're really due from the signers (though notably, the signers have failed to do what they were supposed to at this point). Assuming everyone is behaving correctly and honestly, the value loss would once again be small. If someone is misbehaving, there is potentially high value loss (up to the full signer bond). The misbehavior required is sizeable, in that the funder has to also be able to prevent the signing group from forming; however, if the lot size is high enough and the bonds are correspondingly large enough, this may be enticing.
If we do the lesser of the two, we're really not getting any improvement over always refunding based on the current estimate. If we do the greater of the two, then we're not getting any improvement over refunding the funder value. So those two options aren't really worth it.
Given this, I think staying with the current fee estimate feels like the right next move. I still want to talk about this tomorrow (which we didn't do last time I said it hah) -- @NicholasDotSol do you mind making sure we have this conversation?
uint256 _seized = _d.seizeSignerBonds(); | ||
|
||
/* solium-disable-next-line security/no-send */ | ||
_d.depositOwner().send(_d.keepSetupFee); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use call.value
instead of send?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be protected against reentrancy because of our state checks, so this change makes sense. I think I'll pull it to a separate PR though since there are quite a few instances and they all need to be checked against reentrancy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's gas cost optimization when switching to call.value
Any smart contract that uses transfer() or send() is taking a hard dependency on gas costs by forwarding a fixed amount of gas: 2300.
Our recommendation is to stop using transfer() and send() in your code and switch to using call() instead
https://diligence.consensys.net/blog/2019/09/stop-using-soliditys-transfer-now/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I talked to Martin and Alex today, there's a broader recommendation here that we heavily consider withdrawal vs transfer in several cases. Let's talk about it tomorrow.
@@ -65,6 +65,7 @@ library DepositFunding { | |||
uint256 _bondRequirementSatoshi = _lotSizeSatoshis.mul(_system.getInitialCollateralizedPercent()).div(100); | |||
uint256 _bondRequirementWei = _d.fetchBitcoinPrice().mul(_bondRequirementSatoshi); | |||
|
|||
_d.keepSetupFee = _system.createNewDepositFeeEstimate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's chat about this tomorrow, I think. It feels like just a minor griefing vector
After you discuss it please share your findings here, my thought was also that we should use msg.value
here as @Shadowfiend mentioned.
@@ -103,6 +104,14 @@ library DepositFunding { | |||
block.timestamp > _d.signingGroupRequestedAt.add(TBTCConstants.getSigningGroupFormationTimeout()), | |||
"Signing group formation timeout not yet elapsed" | |||
); | |||
|
|||
// refund the deposit owner the cost to create a new keep at current price levels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at current price levels
This is not the current price level but the level from the moment of keep request creation. Please update this comment after the other discussion is resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will updated once resolved, but still a small chance it resolved the other way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
I just noticed that my last comments were sitting in pendning state and I hadn't submitted them before. Sorry for the delay. |
We do not rely on keep to punish signers in case of setup failure. This is not attributable
uint256 _seized = _d.seizeSignerBonds(); | ||
|
||
/* solium-disable-next-line security/no-send */ | ||
_d.depositOwner().send(_d.keepSetupFee); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, this should be a pull payment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to merge this branch and have us consider the pull payment aspect holistically across the codebase next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This landed in #570.
A recent checke broke this branch. Noteable ITBTCSystme is now used to call createNewDepositFeeEstimate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's roll 'er on in.
closes: #495
Is signer setup fails and
notifySignerSetupFailure
is called successfully, the signer bonds are seized and the funder is refunded. The remainder is returned.might be less than the original; however, since the refund comes from signer
bonds, it cannot be based in how much the funder originally sent in payment,
and is best anchored in a system value.