Skip to content
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

feat: ConfidentialERC20Wrapped/ConfidentialWETH #64

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

PacificYield
Copy link
Contributor

@PacificYield PacificYield commented Nov 20, 2024

This PR adds two Solidity contracts: ConfidentialERC20Wrapped.sol and ConfidentialWETH.sol.

It breaks the coverage and needs a fix to avoid doing so.It breaks because of the use of hardhat_setBalance

@PacificYield PacificYield force-pushed the erc20-wrapper branch 3 times, most recently from 179e3e8 to 99cd560 Compare November 22, 2024 10:51
@PacificYield PacificYield self-assigned this Nov 25, 2024
@PacificYield PacificYield force-pushed the erc20-wrapper branch 3 times, most recently from ff066ce to 830c4b6 Compare December 2, 2024 13:55
@PacificYield PacificYield changed the title feat: EncryptedERC20Wrapped/EncryptedWETH feat: ConfidentialERC20Wrapped/ConfidentialWETH (WIP) Dec 2, 2024
@PacificYield PacificYield added the enhancement New feature or request label Dec 3, 2024
@PacificYield PacificYield marked this pull request as ready for review December 3, 2024 10:11
@PacificYield PacificYield requested a review from jatZama December 3, 2024 10:16
@PacificYield PacificYield changed the title feat: ConfidentialERC20Wrapped/ConfidentialWETH (WIP) feat: ConfidentialERC20Wrapped/ConfidentialWETH Dec 3, 2024
@PacificYield PacificYield force-pushed the erc20-wrapper branch 2 times, most recently from e3d76e4 to 0ec252b Compare December 10, 2024 06:40
@PacificYield PacificYield force-pushed the erc20-wrapper branch 2 times, most recently from e23eb8f to a1cbe5d Compare December 16, 2024 17:11
@jatZama
Copy link
Member

jatZama commented Dec 20, 2024

burn logic seems good, however there is one main issue as discussed on Slack:
For the WrappedETH currently if callback of the unwarp fails in case callSucess is false, it will make the user locked and lose his funds forever.
Current agreed solution is to handle this case differently: if callSuccess is false, instead of reverting the callback, the burn should not happen, but the user should still get unlocked, this was he has an opportunity to transfer his funds to an EOA for instance and then still being able to recover his funds by making a new request.
For the ERC20 Wrapper, there is a very similar issue, so we should add a try catch around the safeTransferFrom for instance.

@jatZama
Copy link
Member

jatZama commented Dec 20, 2024

Also not to forget: remove the duplicate fallback, the receive function is sufficient in the Wrapped ETH.

@jatZama
Copy link
Member

jatZama commented Dec 20, 2024

Also maybe for increased security, it could make sense to allow the user to call an unlockAfterMaxDedline function in case the Gateway was never able to send back a decryption request on time. But since the Gateway design is being reworked now, we could do this in a future version, just something to keep in mind here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants