[DRAFT] fix: prevent BREAD from being credited to the token contract address - #35
Draft
bagelface wants to merge 1 commit into
Draft
[DRAFT] fix: prevent BREAD from being credited to the token contract address#35bagelface wants to merge 1 commit into
bagelface wants to merge 1 commit into
Conversation
bagelface
marked this pull request as draft
August 7, 2026 13:05
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #34.
What changed
BREAD can no longer be credited to the token contract's own address. The check lives in
_update, so it covers every balance-changing path in one place:transfer,transferFrom, bothmintoverloads andclaimYieldnow revert withInvalidRecipient()when the recipient is the token contract. A transfer-only check would have left the mint paths open, which is why the guard sits in_updaterather than in the two public transfer functions.Notes on the issue as filed
Two corrections worth recording:
Tokens sent this way were recoverable, not lost.
rescueToken(address(bread), amount)already worked for them —tok != sexyDaipasses the guard and the contract calls its owntransfer. So this was a low-severity UX and trust problem (owner has to notice, and the funds land at the owner rather than the sender), not permanent loss. Nothing is currently stranded either: the deployed token holds 0 BREAD at its own address. This change is preventative.There was also a quieter side effect: the existing
transferoverride self-delegated the recipient, so a stranded balance also parked voting power on an address that can never vote.Rejecting native xDAI is deliberately not done here. BREAD sits behind
EIP173ProxyWithReceive, whosereceive()accepts xDAI and never delegatecalls the implementation — areceive()/fallback()added toBread.solwould be dead code that reads as protection while changing nothing. It also cannot be done at all without redeploying the proxy, which means a new token address and a holder migration.More importantly, a blanket ETH rejection would break redemptions:
burn()is paid out bywxDai.withdraw()sending xDAI to the contract with empty calldata.test_burn_receives_native_after_recipient_checkpins that behaviour so it can't be regressed later. xDAI sent directly to the token contract therefore stays put; a sweep function can be added in a later upgrade if it's ever needed.Tests
19/19 pass against a Gnosis fork (14 pre-existing, no regressions). Five new tests cover each credit path rejecting, plus the burn/native-receive invariant.
Deploying
Needs a new implementation plus
upgradeTofrom proxy admin0x918dEf5d593F46735f74F9E2B280Fe51AF3A99aD. No new state variables, so storage layout is unchanged and the upgrade is layout-safe.Tradeoffs
_updateis broader than a strict ERC20 reading, which says transfers to any non-zero address should succeed. Any integration that deliberately sends BREAD to the token contract would now revert; none was found, and the yield distribution flow does not do this. Narrowing totransfer/transferFromwould be more conservative but would leave the mint paths open.