Prefer own-storage slots and cache cross-contract overrides per holder in balance-overrides

[jmg-duarte]

Description

Orders buying RSR fail the order-creation simulation with ERC20: transfer amount exceeds balance although they settle fine (~600 shadow rejections per week on mainnet, e.g. order 0x9984fb… on 2026-10-13, which filled on chain).

RSR is copy-on-write over the old RSR contract: balanceOf reads oldRSR until the account's first transfer sets balCrossed, and allowance reads oldRSR.allowance until the pair's first approve. The override detector traced balanceOf for an uncrossed holder, where the old contract's slot also verifies, and cached that strategy for every holder. For the settlement contract, crossed years ago, the override is written to a contract RSR no longer reads, so the payout reverts. The approval detector has the same flaw for crossed (owner, spender) pairs.

flowchart LR
    Q[quote for fresh trader] --> D[detect: trace balanceOf]
    D -->|uncrossed holder| O[old RSR slot verifies]
    O --> C[(cache: token, any holder)]
    S[order simulation: fund settlement] --> C
    C --> W[override written to old RSR]
    W --> X[settlement is crossed: balanceOf ignores it]
    X --> R[payout reverts, order rejected]

Loading

Changes

• Candidate slots in the token's own storage are tried before slots found in other contracts (balance::prefer_own_storage, approval::prefer_own_storage).
• Strategies in another contract are cached per holder, or per (owner, spender) pair, instead of for the whole token (cache_key).
• Forked e2e test forked_node_mainnet_balance_override_copy_on_write_token: detects for an uncrossed account first, then asserts the override still works for the crossed settlement contract and for a real trader's crossed allowance. Fails on main, passes here.

How to test

FORK_URL_MAINNET=<archive rpc> cargo nextest run -p e2e forked_node_mainnet_balance_override \
  --run-ignored ignored-only --test-threads 1
cargo nextest run -p balance-overrides

[claude]

Claude finished @jmg-duarte's task in 4m 8s —— View job

---

PR Review

Solid fix — the prefer_own_storage + per-holder cache_key approach is the right shape for copy-on-write tokens, the reasoning in the comments is excellent, and the forked e2e test captures the settlement (crossed) direction well. One substantive concern below.

Residual order-dependent hole on the allowance side (inline comment): the own-storage mapping is still cached under (token, None), which detect() checks first. For a copy-on-write token, that mapping is only valid for crossed pairs. If a crossed pair is detected before an uncrossed one, every later uncrossed pair gets the own-storage strategy and its allowance override silently no-ops (allowance() reads the old contract) — re-introducing the same shadow-rejection class, just for uncrossed owners. The new test doesn't catch it because it detects the uncrossed pair first. The balance side has the same asymmetry but is benign (additive → over-funds). Worth deciding whether copy-on-write tokens should cache an own-storage mapping globally at all.

Could not run cargo nextest/the forked e2e here (sandbox); review is static.

• Read changed files and understand context
• Review balance override logic
• Review approval override logic
• Review e2e test
• Post inline comments and summary
  · jmgd/rsr-balance-override

[claude]

Residual order-dependent failure for uncrossed pairs (opposite direction of the bug this PR fixes).

For a copy-on-write token, neither the own-storage mapping nor the old-contract mapping is valid for all pairs: own-storage works only for crossed pairs, the old contract only for uncrossed ones. Both are SolidityMappings, so is_valid_for_all_pairs() is true for each. This cache_key correctly demotes the foreign mapping to per-pair, but it still caches the own-storage mapping under (token, None) — and detect() consults (token, None) first, short-circuiting per-pair detection.

So the fix is order-sensitive. If a crossed pair is detected first, it populates (token, None) with the own-storage mapping. Every subsequent uncrossed pair then gets that strategy, writes the override to RSR's own allowance slot, but allowance() still reads oldRSR.allowance for uncrossed pairs → the override has no effect, the allowance stays 0, and the simulation fails with the same class of shadow rejection this PR targets (just for uncrossed owners instead of crossed ones). The new e2e test happens to detect the uncrossed pair first, so it never exercises this path.

You can reproduce it by appending a re-check of the fresh/uncrossed pair after the crossed one in the e2e test:

// after CROSSED_TRADER populates (RSR, None) with the own-storage mapping
assert_allowance_override(&web3, &overrides, fresh_owner, VAULT_RELAYER, amount).await; // fails

The balance side has the analogous asymmetry but is benign: balanceOf is additive (_oldBal + super.balanceOf), so an own-storage override on an uncrossed holder over-funds rather than under-funds. Allowance is a pure replacement, so it hard-fails.

Worth deciding whether copy-on-write tokens should ever cache an own-storage mapping under (token, None) — e.g. cache it per-pair too when a foreign slot was among the candidates, or detect the copy-on-write shape explicitly.

Fix this →

[MartinquaXD]

Hmm, that's quite annoying. Not sure how to best work around that without assuming all tokens have this issues and completely disabling caching altogether.
For context this is the problematic token code which makes the issue more obvious:

    /// The allowance is a combination of crossing allowance + newly granted allowances
    function allowance(address owner, address spender) public view override returns (uint256) {
        if (allowanceCrossed[owner][spender]) {
            return super.allowance(owner, spender);
        }
        return oldRSR.allowance(owner, spender);
    }

Depending on which address we cache the result for first we either store the contract itself or the old contract. But because the contract checks only 1 contract instead of doing a fall through it's an issue.

[MartinquaXD]

Given how little code this function has wouldn't it make more sense to have it be part of find_plausible_storage_slots? That way a caller wouldn't have to remember to compose the functions correctly.
Also the comment on find_plausible_storage_slots already mentions a specific ordering to improve the odds.

[MartinquaXD]

Hmm, that's quite annoying. Not sure how to best work around that without assuming all tokens have this issues and completely disabling caching altogether.
For context this is the problematic token code which makes the issue more obvious:

    /// The allowance is a combination of crossing allowance + newly granted allowances
    function allowance(address owner, address spender) public view override returns (uint256) {
        if (allowanceCrossed[owner][spender]) {
            return super.allowance(owner, spender);
        }
        return oldRSR.allowance(owner, spender);
    }

Depending on which address we cache the result for first we either store the contract itself or the old contract. But because the contract checks only 1 contract instead of doing a fall through it's an issue.