Return the amount of wrapped token sent on wrap calls#307
Conversation
✅ Deploy Preview for confidential-tokens ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThe PR updates the ERC7984ERC20Wrapper contract to modify the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol`:
- Around line 82-93: The wrap function may silently receive a short or zero mint
from _mint (which returns the encrypted amount minted), so update wrap to
validate the returned euint64 wrappedAmountSent against the expected minted
quantity (compute expected as FHE.asEuint64(SafeCast.toUint64(amount / rate()))
or equivalent) and revert if wrappedAmountSent is zero or not equal to the
expected value; ensure you perform this check before calling FHE.allowTransient
and returning, or alternatively upgrade the ERC7984 implementation to >=0.3.1 to
eliminate the silent-failure bug (references: wrap, _mint, wrappedAmountSent,
rate(), FHE.allowTransient).
| * | ||
| * Returns the amount of wrapped token sent. | ||
| */ | ||
| function wrap(address to, uint256 amount) public virtual override { | ||
| function wrap(address to, uint256 amount) public virtual override returns (euint64) { | ||
| // take ownership of the tokens | ||
| SafeERC20.safeTransferFrom(IERC20(underlying()), msg.sender, address(this), amount - (amount % rate())); | ||
|
|
||
| // mint confidential token | ||
| _mint(to, FHE.asEuint64(SafeCast.toUint64(amount / rate()))); | ||
| euint64 wrappedAmountSent = _mint(to, FHE.asEuint64(SafeCast.toUint64(amount / rate()))); | ||
| FHE.allowTransient(wrappedAmountSent, msg.sender); | ||
|
|
||
| return wrappedAmountSent; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the main ERC7984 contract file
fd -a '^ERC7984\.sol$' --type f contracts/Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 343
🏁 Script executed:
#!/bin/bash
# Find and examine _mint implementation
rg -n "function _mint" contracts/token/ERC7984/ -A 10Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 343
🏁 Script executed:
#!/bin/bash
# Also check _update implementation since _mint often delegates to it
rg -n "function _update" contracts/token/ERC7984/ -A 10Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 343
🏁 Script executed:
#!/bin/bash
# Check the wrap function documentation more thoroughly
cat -n contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol | sed -n '75,95p'Repository: OpenZeppelin/openzeppelin-confidential-contracts
Length of output: 343
🌐 Web query:
ERC7984 _mint return semantics OpenZeppelin confidential contracts
💡 Result:
In OpenZeppelin Confidential Contracts (@openzeppelin/confidential-contracts), ERC7984’s internal mint is not “ERC20-style (bool success / revert)”. Its signature is:
function _mint(address to, euint64 amount) internal returns (euint64 transferred)— it returns the encrypted amount actually minted (“transferred”), not a boolean. [1]
Return semantics (important):
- The
transferredreturn value can be less than requested, including0. - In particular, in versions <= 0.3.0, if minting would make the confidential
euint64total supply overflow,_mintcan fail silently (i.e., not revert) and effectively mint0, so callers must not assume mint succeeded unless they check the returnedtransferred. This behavior is the root cause of GHSA-hqf9-8xv5-x8xw. [2]
Practical implication:
- If your logic needs “mint must succeed or revert” (e.g., wrappers), you should check the returned
euint64 transferred(or upgrade to >= 0.3.1, which patches the wrapper-side issue). [2]
Sources: OpenZeppelin ERC7984 API docs [1], OpenZeppelin advisory GHSA-hqf9-8xv5-x8xw (patched in 0.3.1) [2].
The wrap function must handle potential silent mint failures.
_mint in ERC7984 returns the encrypted amount actually minted (not the updated balance), so the return semantics are correct. However, in versions <= 0.3.0, _mint can silently fail to mint the full amount or return 0 due to overflow, without reverting. The wrap function should either:
- Validate that
wrappedAmountSentequals the requested amount (or handle underflow) - Upgrade to version >= 0.3.1, which patches this issue
- Or explicitly document this failure mode for callers
🤖 Prompt for AI Agents
In `@contracts/token/ERC7984/extensions/ERC7984ERC20Wrapper.sol` around lines 82 -
93, The wrap function may silently receive a short or zero mint from _mint
(which returns the encrypted amount minted), so update wrap to validate the
returned euint64 wrappedAmountSent against the expected minted quantity (compute
expected as FHE.asEuint64(SafeCast.toUint64(amount / rate())) or equivalent) and
revert if wrappedAmountSent is zero or not equal to the expected value; ensure
you perform this check before calling FHE.allowTransient and returning, or
alternatively upgrade the ERC7984 implementation to >=0.3.1 to eliminate the
silent-failure bug (references: wrap, _mint, wrappedAmountSent, rate(),
FHE.allowTransient).
3 participants
The amount wrapped is readily available to the wrap function--we should return it with transient approval for integrators.
Summary by CodeRabbit