Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

Refund funder if signer setup fails #520

Merged
merged 15 commits into from Mar 27, 2020
Merged

Refund funder if signer setup fails #520

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
73b8071
Refund funder if signer setup fails
Mar 12, 2020
2592f66
Update notifySignerSetupFailure test
Mar 12, 2020
3a9b209
Merge branch 'master' into sig-err-refund
NicholasDotSol Mar 12, 2020
8bb11b6
disable solidum warning on send()
Mar 12, 2020
2ff2bdf
Only use real TBTCSystem after notifySignerSetupFailure
Mar 12, 2020
21e8ac2
Merge branch 'sig-err-refund' of https://github.com/keep-network/tbtc…
Mar 12, 2020
a8897a8
Merge branch 'master' into sig-err-refund
NicholasDotSol Mar 16, 2020
66e4786
Track Keep setup fee
Mar 16, 2020
aaac391
Updates notifySignerSetupFailure test
Mar 16, 2020
3fb90d6
Remove whitespace before parenthesis
Mar 16, 2020
17427a8
Merge branch 'master' into sig-err-refund
Mar 26, 2020
555fe11
Remove outdated comment
Mar 26, 2020
0c75e04
Merge branch 'master' into sig-err-refund
Shadowfiend Mar 27, 2020
324c7a0
Fix compiiler error
Mar 27, 2020
ad2d385
Update outdated in-line comment
Mar 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions implementation/contracts/deposit/DepositFunding.sol
View file
Open in desktop
Expand Up @@ -65,6 +65,7 @@ library DepositFunding {
uint256 _bondRequirementSatoshi = _lotSizeSatoshis.mul(_system.getInitialCollateralizedPercent()).div(100);
uint256 _bondRequirementWei = _d.fetchBitcoinPrice().mul(_bondRequirementSatoshi);

_d.keepSetupFee = _system.createNewDepositFeeEstimate();
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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?

/* solium-disable-next-line value-in-payable */
_d.keepAddress = _system.requestNewKeep.value(msg.value)(_m, _n, _bondRequirementWei);
_d.signerFeeDivisor = _system.getSignerFeeDivisor();
Expand Down Expand Up @@ -103,6 +104,14 @@ library DepositFunding {
block.timestamp > _d.signingGroupRequestedAt.add(TBTCConstants.getSigningGroupFormationTimeout()),
"Signing group formation timeout not yet elapsed"
);

nkuba marked this conversation as resolved.
Show resolved Hide resolved
// refund the deposit owner the cost to create a new keep at current price levels.
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Shadowfiend marked this conversation as resolved.
Show resolved Hide resolved
uint256 _seized = _d.seizeSignerBonds();

/* solium-disable-next-line security/no-send */
_d.depositOwner().send(_d.keepSetupFee);
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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/

Copy link
Contributor

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.

_d.pushFundsToKeepGroup(_seized.sub(_d.keepSetupFee));

_d.setFailedSetup();
_d.logSetupFailed();

Expand Down
1 change: 1 addition & 0 deletions implementation/contracts/deposit/DepositUtils.sol
View file
Open in desktop
Expand Up @@ -35,6 +35,7 @@ library DepositUtils {
uint256 signerFeeDivisor;
uint128 undercollateralizedThresholdPercent;
uint128 severelyUndercollateralizedThresholdPercent;
uint256 keepSetupFee;

// SET ON FRAUD
uint256 liquidationInitiated; // Timestamp of when liquidation starts
Expand Down
4 changes: 4 additions & 0 deletions implementation/contracts/test/deposit/TestDeposit.sol
View file
Open in desktop
Expand Up @@ -63,6 +63,10 @@ contract TestDeposit is Deposit {
self.signerFeeDivisor = _signerFeeDivisor;
}

function setKeepSetupFee(uint256 _fee) public {
self.keepSetupFee = _fee;
}

function getSignerFeeDivisor() public view returns (uint256) { return self.signerFeeDivisor; }

function setLotSize(uint256 _lotSizeSatoshis) public {
Expand Down
38 changes: 36 additions & 2 deletions implementation/test/DepositFundingTest.js
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@ const {BN, constants, expectRevert} = require("@openzeppelin/test-helpers")
const {ZERO_ADDRESS} = constants
const {expect} = require("chai")
const ECDSAKeepStub = contract.fromArtifact("ECDSAKeepStub")
const TBTCSystem = contract.fromArtifact("TBTCSystem")

// spare signature:
// signing with privkey '11' * 32
Expand Down Expand Up @@ -50,6 +51,7 @@ describe("DepositFunding", async function() {
let tbtcDepositToken
let testDeposit
let ecdsaKeepStub
let ecdsaKeepFactory

let fundingProofTimerStart
let beneficiary
Expand All @@ -66,8 +68,10 @@ describe("DepositFunding", async function() {
tbtcDepositToken,
testDeposit,
ecdsaKeepStub,
ecdsaKeepFactoryStub,
} = await deployAndLinkAll())

ecdsaKeepFactory = ecdsaKeepFactoryStub
beneficiary = accounts[4]
await tbtcDepositToken.forceMint(
beneficiary,
Expand Down Expand Up @@ -171,29 +175,59 @@ describe("DepositFunding", async function() {
)
})
})

describe("notifySignerSetupFailure", async () => {
let timer
let owner
let openKeepFee
before(async () => {})

before(async () => {
;({
tbtcConstants,
mockRelay,
tbtcSystemStub,
tbtcToken,
tbtcDepositToken,
testDeposit,
ecdsaKeepStub,
} = await deployAndLinkAll([], {TBTCSystemStub: TBTCSystem}))

openKeepFee = await ecdsaKeepFactory.openKeepFeeEstimate.call()
await testDeposit.setKeepSetupFee(openKeepFee)
owner = accounts[1]
await tbtcDepositToken.forceMint(
owner,
web3.utils.toBN(testDeposit.address),
)
timer = await tbtcConstants.getSigningGroupFormationTimeout.call()
})

beforeEach(async () => {
const block = await web3.eth.getBlock("latest")
const blockTimestamp = block.timestamp
const value = openKeepFee + 100

await ecdsaKeepStub.send(value)

fundingProofTimerStart = blockTimestamp - timer.toNumber() - 1

await testDeposit.setState(states.AWAITING_SIGNER_SETUP)

await testDeposit.setFundingProofTimerStart(fundingProofTimerStart)
})

it("updates state to setup failed, deconstes state, and logs SetupFailed", async () => {
it("updates state to setup failed, deconstes state, logs SetupFailed, and refunds TDT owner", async () => {
const initialFunderBalance = await web3.eth.getBalance(owner)
const blockNumber = await web3.eth.getBlock("latest").number
await testDeposit.notifySignerSetupFailure()

const signingGroupRequestedAt = await testDeposit.getSigningGroupRequestedAt.call()
const finalFunderBalance = await web3.eth.getBalance(owner)

expect(
new BN(finalFunderBalance).sub(new BN(initialFunderBalance)),
).to.eq.BN(openKeepFee)

expect(
signingGroupRequestedAt,
"signingGroupRequestedAt should be 0",
Expand Down