-
Notifications
You must be signed in to change notification settings - Fork 96
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.
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
multi: ETH Fidelity Bonds #2702
Conversation
martonp
commented
Feb 14, 2024
This adds a solidity contract for managing ETH fidelity bonds. In addition to the usual functionalities of creating and refunding bonds, it also allows using funds locked in existing bonds to create a new bond. If a bond with more funds than the existing bonds is being created, the user has to send additional funds, and if a bond is being created with less funds than the existing bonds, a "change" bond is also created with the remainder that maintains the earliest lockTime of the existing bonds. This process will only allow the user to lock funds for a longer period than they were previously locked for, never less. A Hardhat project is also created for this contract to allow easy testing of the smart contract logic. To run the tests, go to the `dex/networks/eth/bondcontracts/v0` folder and run the following: `npm install` `npx hardhat test`
…lient Adds a BondUpdater interface which contains a single method, `UpdateBondsTx`. Similar to MakeBondTx, this generates an unsigned transaction, but instead of creating a new bond from scratch, it uses the funds locked in existing bonds to create a new bond. For ETH, it is not possible to create a signed transaction that will be submitted later because if the nonce is used before the transaction is submitted, the transaction will no longer be valid. Therefore, for ETH, the SignedTx and UnsignedTx in the `asset.Bond` returned from `MakeBondTx` and `UpdateBondsTx` contain the same data. The ETH wallet backend populates the nonce and signs the transaction on the call to `SendTransaction`.
This updates the client to support wallets that implement `BondUpdater`. In `rotateBonds`, if an account is set to maintain a certain tier, and it already has some weak or expired bonds posted in an asset that supports updating bonds, it will use the funds in those bonds to create a new bond with a sufficiently long lockTime.
This adds a BondUpdater interface in the server that requires a backend to implement an `AllAccountBonds` method. in addition to the Bonder interface's methods. This method returns all the currenlty active bonds for an account. This new interface is implemented by the ETH backend. This is required for assets where it is possible to use existing unexpired bonds to fund new bonds, because the server needs to know if bonds that it was previously using the calculates a user's tier no longer exist.
When a new bond is posted for an asset that implements `BondUpdater`, all of the currently active bonds for that asset are retrieved from the blockchain, and the the bonds for this asset that were stored in the database and the `AuthManager` are replaced by what was retreived.
The transaction details popup now shows change and replaced bonds. Also in many placed the bond amount was multiplied by two to handle the bond reserves requirement which bond updaters do not have, so this needed to be updated.
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.
Review of the first commit. Thank you for breaking up the changes like this.
// actual = 59451 | ||
UpdateBondGas uint64 = 400000 |
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.
Almost 7 times actual.
} | ||
|
||
if decoded.Name != "updateBonds" { | ||
return [32]byte{}, nil, nil, nil, 0, err |
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.
err is still nil here, need to make a custom one
} | ||
|
||
if decoded.Name != "createBond" { | ||
return [32]byte{}, [32]byte{}, 0, fmt.Errorf("expected method 'newBond', got '%s'", decoded.Name) |
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.
err name says "newBond" but it was looking for "createBond"
|
||
const numArgs = 5 | ||
if len(args) != numArgs { | ||
return [32]byte{}, nil, nil, nil, 0, err |
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.
err is nil
// firstBondID is the bondID of the first bond in the doubly-linked list. | ||
bytes32 firstBondID; |
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.
Does first mean latest?
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.
The doubly-linked list is just to make it possible to check all the bonds for an account. It doesn't really matter what order they are in.
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, but the name is confusing in my opinion. I think first means first in a sequence, like the very first. But this changes every new bond is is the latest or last.
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 just meant the first element of the list. I can make it headBondID
.
|
||
(bool valid, uint totalValue, string memory reason) = verifyBondLocktimesAndOwner(accountBonds, bondsToUpdate, locktime); | ||
if (!valid) { | ||
return (false, locktime, reason); |
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.
Should return total value rather than locktime?
previousLocktime = bond.locktime; | ||
} | ||
|
||
Bond storage latestExpiringBond = accountBonds[bondsToUpdate[bondsToUpdate.length - 1]]; |
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.
Should this check that the length was > 0?
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.
No, this updateBonds
shouldn't be called without any bonds to update.
return !((v == nil || v.Cmp(big.NewInt(0)) == 0) && | ||
(r == nil || r.Cmp(big.NewInt(0)) == 0) && | ||
(s == nil || s.Cmp(big.NewInt(0)) == 0)) |
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.
May be slightly more efficient and straightforward to return true if any are not zero rather than always checking all three and inverting. Comment would be "If any values are non zero the tx is signed."
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.
Review for the second commit.
accountID, bondID, lockTime, err := dexeth.ParseCreateBondTx(tx.Data()) | ||
if err == nil { | ||
return asset.CreateBond, newBondTxInfo(accountID, bondID, lockTime) | ||
} | ||
|
||
accountID, bondsToUpdate, newBondIDs, value, locktime, err := dexeth.ParseUpdateBondsTx(tx.Data()) | ||
if err == nil { | ||
return asset.UpdateBond, updateBondTxInfo(accountID, bondsToUpdate, newBondIDs, value, locktime) | ||
} |
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.
Ok to ignore error contents? If the error does not matter maybe change to bool? Or use errors with types if we care about logging some? The log below ignores the first error.
if newBondErr != nil { | ||
_, _, _, _, _, updateBondErr := dexeth.ParseUpdateBondsTx(tx.Data()) | ||
if updateBondErr != nil { | ||
return nil, fmt.Errorf("non-bond creation transactions must be signed before sending: %w", newBondErr) |
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.
Is this error correct? Also ignoring updateBondErr contents.
// transactions cannot be signed beforehand due to nonce ordering, so the | ||
// the SignedTx and UnsignedTx fields in the return value both contain |
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.
// transactions cannot be signed beforehand due to nonce ordering, so the | |
// the SignedTx and UnsignedTx fields in the return value both contain | |
// transactions cannot be signed beforehand due to nonce ordering, so the | |
// SignedTx and UnsignedTx fields in the return value both contain |
|
||
var accountID, bondID [32]byte | ||
copy(accountID[:], acctID) | ||
copy(bondID[:], privKey.Serialize()) |
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.
should use the pubkey hash like utxo coins? This private key can be read by others on chain right? This leaks private info from core. Unsure if this is a problem for account bonds but just to be safe...
I've forgotten if the server needs this for verification. For the current contract this is the key, but I think I suggested that just be a nonce. It could be any random number though correct? This passed value doesn't have to be used at all?
For utxo coins it is used so that we can refund the bond tx with any wallet. So the user could make an internal wallet and bond and then switch to an rpc or electrum wallet and refund that bond. Are eth users able to use different wallets like that with the current code? I guess currently the account is always imported into the new wallet so it is never a problem? There can only be the wallet from the internal seed? Maybe worth noting this distinction somewhere so we don't break eth bonds accidentally...
I realize with the current contract anyone can refund but the funds can only go to the original bonding wallet and another wallet cannot use the update function because previous bond owner is checked. You could use the bond key though I think, if you wanted to.
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.
should use the pubkey hash like utxo coins? This private key can be read by others on chain right? This leaks private info from core. Unsure if this is a problem for account bonds but just to be safe...
I've forgotten if the server needs this for verification. For the current contract this is the key, but I think I suggested that just be a nonce. It could be any random number though correct? This passed value doesn't have to be used at all?
I don't think there's any security risk, but can't hurt to hash it. And yes, it can be just any random number. If we do a nonce, then each account has to maintain a nonce counter.. I don't see the point of that. Might as well just pass random numbers.
For utxo coins it is used so that we can refund the bond tx with any wallet. So the user could make an internal wallet and bond and then switch to an rpc or electrum wallet and refund that bond. Are eth users able to use different wallets like that with the current code? I guess currently the account is always imported into the new wallet so it is never a problem? There can only be the wallet from the internal seed? Maybe worth noting this distinction somewhere so we don't break eth bonds accidentally...
Yes, you will have to refund to the same address. It's 30k gas to do an additional ECDSA verification, so I don't think that's worth it to allow refunding to other wallets. As long as they don't lose the dexc seed, they'll be fine.
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.
It's 30k gas to do an additional ECDSA verification, so I don't think that's worth it to allow refunding to other wallets.
I won't push it because I don't know if we will need it but you could make another function likerefundToAnyAddress
that checks they can sign some info using the bond key and then sends the refund to any specified address. Then the normal path can stay cheap.
totalBondValueWei := big.NewInt(0) | ||
|
||
for i, bondCoin := range bondCoins { | ||
bondCoin, err := decodeBondCoin(bondCoin) |
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 surprised you can save a struct to the dex.Bytes type here... They are both called bondCoin and in the same scope right? No need to change just guess I don't understand the language as well as I thought.
copy(accountID[:], coinID[0:32]) | ||
copy(bondID[:], coinID[32:64]) |
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.
nit:
copy(accountID[:], coinID[0:32]) | |
copy(bondID[:], coinID[32:64]) | |
copy(accountID[:], coinID[:32]) | |
copy(bondID[:], coinID[32:]) |
b := encode.BuildyBytes{0} | ||
b := encode.BuildyBytes{1} |
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.
Could note that this is the version. Even give it a name? 0 is initial version and now 1 is removeAfterConfirmsVersion
if ver > 1 { | ||
return fmt.Errorf("unknown version %d", ver) | ||
} | ||
|
||
if ver == 0 { | ||
return m.unmarshalV0(pushes) | ||
} | ||
|
||
return m.unmarshalV1(pushes) |
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.
Might read better as a switch with cases. Is easier to add versions as well imo. Also giving the versions names might be good.
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.
Great work! Thank you again for breaking down the commits, it was very easy to review.
My main concern is that the contract can be abused currently by not telling the server everything. Maybe the answer is that if you combine bonds, the "change bond" must have a locktime of the latest bond you are updating, that way no value can be siphoned out using earlier bonds.
I could also be wrong there, I have not tried any tests to prove this yet.
I also had a few comments about the bond id which is private info that is not really used to its full potential. Unlike utxo coins you cannot refund to a new account. Also the private data is viewable on the ethereum chain, which probably is not good. At least you should use the pubkey hash imo.
// If we reusing more than the size of the new bond, we need | ||
// a "change" bond. |
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 we reusing more than the size of the new bond, we need | |
// a "change" bond. | |
// If we are reusing more than the size of the new bond, we need | |
// a "change" bond. |
} | ||
return decodeBond_v3(append(pushes, nil /* replacedBy */)) | ||
} | ||
func decodeBond_v3(pushes [][]byte) (*Bond, error) { |
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.
Maybe a small note as to what changed?
if err != nil { | ||
log.Warnf("Error sending postbond result to user %v: %v", acctID, err) | ||
if err = auth.Send(acctID, resp); err != nil { | ||
log.Warnf("Error sending feepaid notification to account %v: %v", acctID, err) |
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.
feepaid -> post bond
@JoeGruffins The rule is, the bonds to update must be passed in increasing locktime order. The change bond must have less value than the earliest expiring bond, and the same locktime. The server doesn't need to know anything other the fact that you have done an update. Then the server goes to the contract and checks every single bond for your account. |
The value rule:
After some more staring I think I understand. The entire change value must be the same or lower than the earliest bond, so the value of the lowest bond, or change, cannot go up at all. So the remaining can only be extended. Thanks for explaining it again. |
// BondTxInfo contains information about a CreateBond or RedeemBond | ||
// transaction. | ||
type BondInfo struct { | ||
ID string `json:"id"` |
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.
Why not dex.Bytes
?
@@ -832,6 +846,18 @@ func (w *ETHWallet) Connect(ctx context.Context) (_ *sync.WaitGroup, err error) | |||
} | |||
} | |||
|
|||
bondAddress, found := dexeth.ETHBondAddress[w.net] |
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.
Polygon will use this connect method too, and we should only be implementing Bonder
when we have a bond address.
Please consider this refactor to get the interfaces worked out. buck54321/dcrdex@f374f4c...buck54321:dcrdex:bonder
} | ||
|
||
// bondReserves returns the bond fee buffer if the nominal bond reserves | ||
// and greater than zero, otherwise it returns zero. |
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.
and -> are
if baseWallet.emit == nil { | ||
return | ||
} |
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.
Why tho? (2 places)
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 is testing code so I don't think we need to commit the package-lock.
// Bond holds information about a single bond. | ||
struct Bond { | ||
uint value; | ||
uint initBlockNumber; | ||
uint64 locktime; | ||
address owner; | ||
|
||
// prevBondID and nextBondID are used to create a doubly-linked list | ||
// of bonds for an account. This is needed to be able to lookup all of | ||
// the bonds for an account. | ||
bytes32 prevBondID; | ||
bytes32 nextBondID; | ||
} |
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 is a lot of data to store. What about a (completely untested) construction like this...
// SPDX-License-Identifier: BlueOak-1.0.0
pragma solidity = 0.8.19;
contract ETHBond {
struct Bond {
uint256 value;
uint64 locktime;
}
mapping(bytes32 => bytes32) bondCommits;
function hashBond(Bond calldata bond) public pure returns (bytes32) {
return sha256(bytes.concat(bytes32(bond.value), bytes8(bond.locktime)));
}
// hashBonds creates a hash that commits to the sender and the provided bonds.
function hashBonds(Bond[] calldata bonds) public view returns (bytes32) {
bytes32 h;
for (uint i = 0; i < bonds.length; i++) {
if (i == 0) {
h = sha256(bytes.concat(bytes20(msg.sender), hashBond(bonds[i])));
} else {
h = sha256(bytes.concat(h, hashBond(bonds[i])));
}
}
return h;
}
function updateBonds(bytes32 acctID, Bond[] calldata oldBonds, Bond[] calldata newBonds) external payable {
bytes32 commit = bondCommits[acctID];
if (uint256(commit) != 0) {
require(oldBonds.length > 0, "need old bonds");
require(hashBonds(oldBonds) == commit, "wrong old bonds or sender");
} else {
require(oldBonds.length == 0, "old bonds provided but no commit is known");
}
// Quick path for pure refund.
if (newBonds.length == 0) {
uint256 refund = 0;
for (uint i = 0; i < oldBonds.length; i++) {
require(oldBonds[i].locktime < block.timestamp, "cannot refund unexpired bond");
refund += oldBonds[i].value;
}
(bool ok, ) = payable(msg.sender).call{value: refund}("");
require(ok == true, "full refund failed");
bondCommits[acctID] = bytes32(0);
return;
}
uint64 stamp = newBonds[newBonds.length - 1].locktime;
uint256 newFunds = 0;
Bond[] memory bondReserves = oldBonds;
// Fund new bonds.
for (uint i = newBonds.length - 1; i >= 0; i--) {
Bond memory newBond = newBonds[i];
require(newBond.locktime <= stamp, "bonds not sorted");
stamp = newBond.locktime;
uint256 toFund = newBond.value;
for (uint j = bondReserves.length - 1; j >= 0; j--) {
require(bondReserves[j].locktime < newBond.locktime, "illegal lock time shortening");
if (bondReserves[j].value >= toFund) {
bondReserves[j].value -= toFund;
toFund = 0;
break;
} else {
toFund -= bondReserves[j].value;
bondReserves[j].value = 0;
}
}
newFunds += toFund;
}
// Are bonds not funded by extending old bonds funded by the transaction?
require (msg.value >= newFunds, "unfunded bonds");
// If some bonds weren't extended, refund them;
uint256 unextended = 0;
for (uint j = bondReserves.length - 1; j >= 0; j--) {
if (bondReserves[j].value > 0) {
require(bondReserves[j].locktime < block.timestamp, "unextended bond is not old enough to refund");
unextended += bondReserves[j].value;
}
}
if (unextended > 0) {
(bool ok, ) = payable(msg.sender).call{value: unextended}("");
require(ok == true, "unextended refund failed");
}
bondCommits[acctID] = hashBonds(newBonds);
return;
}
function validateBonds(bytes32 acctID, Bond[] calldata bonds) external view returns (bool) {
return bondCommits[acctID] == hashBonds(bonds);
}
}
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 we were concerned about recovering bonds, It could also be tweaked to something like.
struct BondCommit {
bytes32 commitHash;
uint256 totalBonded;
uint updateBlockNumber;
address bonder;
uint64 ultimateLocktime; // highest locktime of all bonds
}
mapping(bytes32 => BondCommit) bondCommits;
and then add recovery methods. This should still save a large amount on gas.
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 is good.
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.
With this code anyone could lock down an accountID that does not belong to them, if that is a concern.
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.
The user has the private key for the account id don't they? Seems they could sign a hash of the info they are sending and send the signature too. The contract could verify the signature and then the owner of the id could refund anywhere and such as only they could touch thier account id bonds.
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.
With this code anyone could lock down an accountID that does not belong to them, if that is a concern.
Yeah this is not ideal.
The user has the private key for the account id don't they? Seems they could sign a hash of the info they are sending and send the signature too. The contract could verify the signature and then the owner of the id could refund anywhere and such as only they could touch thier account id bonds.
The account ID is the blake256 hash of the public key, and Ethereum does not support blake256, so I'm not sure how to solve this.
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.
Maybe just allow multiple commits per account ID.
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.
hashBonds
commits to the msg.sender
, so only the user who creates the bonds can update the bonds
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.
Oh. You're saying somebody could create bonds for an account ID that they don't own and prevent someone else from doing that. Mmmkay. The mapping
key should commit to both account ID and address I guess. Might have to provide the server with more info though.
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 about having a mapping(bytes32 => bytes32[])
. Then each account ID could have multiple commits. Generally only the first one would be used though, unless someone's messing around.
Closing in favor of #2717. |