eth: remove isRedeemable contract method, disable estimateRedeemGas #2111
Conversation
|
Redeployed Goerli contracts, but dex-test server is still expecting the old ones! |
|
New contracts are now in use on dex-test. re-test on goerli |
|
Caution! Still using the bonkers gas limits
Limits are revised up in #2013. I'll rebase that PR on this after redeploying on mainnet as well. |
client/asset/eth/eth.go
Outdated
| /* We could get a gas estimate via RPC, but this will reveal the secret key | ||
| before submitting the redeem transaction. This is not OK for maker. This | ||
| could be acceptable if any error from estimateRedeemGas is not fatal, and | ||
| we redeem regardless (using hard-coded gas). Disable for now. |
There was a problem hiding this comment.
This could be acceptable if any error from estimateRedeemGas is not fatal, and we redeem regardless
I question whether even that is appropriate. If the risk is that the provider is in on the scam, then they could just deny our redemption broadcast.
There was a problem hiding this comment.
I agree with that. I'll revise the comment so it doesn't suggest it as a possibility.
There was a problem hiding this comment.
I do still want to leave this commented code in case we one day have a privacy-preserving client wallet.🤞
Not really. I see that as a proof of concept. Could really just delete it, let it be in git history for reference. |
This method made more sense when the client was using a private node, however this is a vulnerability with public RPC providers. This removes the isRedeemable public method from the Solidity contracts ETHSwapV0 and ERC20SwapV0. This also updates the solc version to 0.8.18, the current latest. This may or may not be required, but there are two solc bugs prior to 0.8.17: - https://etherscan.io/solcbuginfo?a=StorageWriteRemovalBeforeConditionalTermination - https://etherscan.io/solcbuginfo?a=AbiReencodingHeadOverflowWithStaticArrayCleanup Since we are having to re-deploy the contracts, we are updating the solc version to 0.8.18+commit.87f61d96 (*assetWallet).isRedeemable now performs the redeemable check itself. That is: - the swap() contractor method can retrieve the swap state - the swap state is SSInitiated - the secret hashes to the secretHash
chappjc
force-pushed
the
sol-isredeemable
branch
from
8ddb5b9
to
132aefd
Compare
The
isRedeemablemethod made more sense when the client was using a private node, however this is a vulnerability with public RPC providers, which is what we are forced to use since The Merge (boo).This removes the
isRedeemablepublic method from the Solidity contractsETHSwapV0andERC20SwapV0.This also updates the solc version to 0.8.18 (github download), the current latest. This may or may not be required, but there are two solc bugs prior to 0.8.17:
Since we are having to re-deploy the contracts, we are updating the solc version to 0.8.18+commit.87f61d96. Note that
abigenv1.10.26 was used to regenerate the contract ABI bindings.(*assetWallet).isRedeemablenow performs the redeemable check itself. That is:swap()contractor method can retrieve the swap stateSSInitiatedsecrethashes to thesecretHashFinally this disables the use of
estimateRedeemGas, which also reveals secret ahead of the redeem transaction. There is a possibly we could use this method if we recognize a private full node as the backend, or if we ensure thatredeemalways occurs even ifestimateRedeemGasfails or returns nonsense. Further, this would be safe for taker (participant), but not maker (initiator).