Skip to content

feat: Total Balance change enforcers#117

Merged
McOso merged 31 commits intomainfrom
chore/balance-payment-enforcement
Jul 25, 2025
Merged

feat: Total Balance change enforcers#117
McOso merged 31 commits intomainfrom
chore/balance-payment-enforcement

Conversation

@hanzel98
Copy link
Copy Markdown
Contributor

@hanzel98 hanzel98 commented Jun 4, 2025

What?

  • Added 4 new enforcers:
    ERC20TotalBalanceChangeEnforcer
    ERC721TotalBalanceChangeEnforcer
    ERC1155TotalBalanceChangeEnforcer
    NativeTotalBalanceChangeEnforcer
    These enforcers can enforce balance change across a delegation chain where multiple same enforcers are present.
  • Included deployment script for all of them

Why?

  • Current balance change enforcers balance changes by comparing the recipient's balance before and after execution. Since enforcers watching the same recipient share state, a single balance modification may satisfy multiple enforcers simultaneously. This can lead to unintended behavior in delegation chains. The total balance change enforcers are introduce to address this issue.

How?

  • Using beforeAllHook and afterAllHook: These hooks enable proper handling of inner delegations and ensure all enforcers in the chain are processed together.
  • Accumulating Expected Changes: Each enforcer maintains a BalanceTracker struct that accumulates the expected increases and decreases for a specific recipient + token combination across all enforcers in the delegation chain.
  • State Isolation: The hash key is generated using the delegation manager address and recipient (plus token address and token ID for ERC1155), ensuring that different delegation managers don't interfere with each other.
  • Aggregated Validation: The final validation in afterAllHook combines all expected changes and validates the total net change against the actual balance change.

@hanzel98 hanzel98 requested a review from a team as a code owner June 4, 2025 20:20
@hanzel98 hanzel98 marked this pull request as draft June 4, 2025 20:20
@hanzel98 hanzel98 changed the title chore: balance payment problematic scenarios [In progress] Balance Payment Problematic Scenarios Jun 9, 2025
@MoMannn MoMannn changed the title [In progress] Balance Payment Problematic Scenarios [In progress] Balance change enforcer Jun 12, 2025
@MoMannn MoMannn changed the title [In progress] Balance change enforcer [In progress] Balance change enforcers Jun 12, 2025
Comment thread src/enforcers/ERC20BalanceChangeEnforcer.sol Outdated
Comment thread src/enforcers/ERC20BalanceChangeEnforcer.sol Outdated
Comment thread src/enforcers/ERC20BalanceChangeEnforcer.sol Outdated
Comment thread src/enforcers/ERC20BalanceChangeEnforcer.sol Outdated
Comment thread src/enforcers/ERC20BalanceChangeEnforcer.sol Outdated
Comment thread src/enforcers/ERC20BalanceChangeEnforcer.sol Outdated
Comment thread src/enforcers/ERC20BalanceChangeEnforcer.sol Outdated
MoMannn and others added 3 commits June 13, 2025 10:11
Co-authored-by: Hanzel Anchia Mena <33629234+hanzel98@users.noreply.github.com>
@hanzel98
Copy link
Copy Markdown
Contributor Author

@MoMannn Please add the deployment script for the new caveats that you create, and add them to the verification scripts too

@hanzel98
Copy link
Copy Markdown
Contributor Author

Please add any documentation needed to explain how the enforced outcomes can be achieved with this enforcer and what is expected when people don't want to use Gators.

Also please explain how the total enforcer works when mixing decrease and increase

Comment thread src/enforcers/ERC20TotalBalanceChangeEnforcer.sol
Comment thread test/enforcers/ERC1155TotalBalanceChangeEnforcer.t.sol
Comment thread test/enforcers/ERC20TotalBalanceChangeEnforcer.t.sol Outdated
@hanzel98
Copy link
Copy Markdown
Contributor Author

Please add a brief explanation comment in each of the test

Comment thread test/enforcers/ERC20TotalBalanceChangeEnforcer.t.sol
Comment thread test/enforcers/ERC1155TotalBalanceChangeEnforcer.t.sol
Comment thread test/enforcers/NativeTotalBalanceChangeEnforcer.t.sol
@MoMannn MoMannn changed the title Balance change enforcers feat: Total Balance change enforcers Jun 20, 2025
@hanzel98 hanzel98 added the new release This PR goes to the next release label Jun 25, 2025
cursor[bot]

This comment was marked as outdated.

Copy link
Copy Markdown
Member

@McOso McOso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Just a little check to short circuit if the amount in terms is greater than 0. Not critical, but might prevent some improper use

Comment thread src/enforcers/ERC20TotalBalanceChangeEnforcer.sol
Comment thread src/enforcers/ERC1155TotalBalanceChangeEnforcer.sol
Comment thread src/enforcers/ERC721TotalBalanceChangeEnforcer.sol
Comment thread src/enforcers/NativeTokenTotalBalanceChangeEnforcer.sol
@MoMannn MoMannn requested a review from McOso July 22, 2025 08:10
cursor[bot]

This comment was marked as outdated.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Script Fails Verifying Contracts at Zero Addresses

The script/verification/verify-enforcer-contracts.sh script adds four new Total Balance Change Enforcers to the ENFORCERS array and corresponding placeholder zero addresses (0x0000000000000000000000000000000000000000) to the ADDRESSES array. The script then attempts to verify contracts at these zero addresses, which causes verification failures.

script/verification/verify-enforcer-contracts.sh#L92-L96

"0x92Bf12322527cAA612fd31a0e810472BBB106A8F"
"0x0000000000000000000000000000000000000000"
"0x0000000000000000000000000000000000000000"
"0x0000000000000000000000000000000000000000"
"0x0000000000000000000000000000000000000000"

Fix in CursorFix in Web


Bug: Unreachable Hooks and Missing Prank Directive

Multiple afterAllHook calls are unreachable as they immediately follow an expectRevert statement. Additionally, one of these unreachable calls is missing a vm.prank(dm) directive, which would lead to an incorrect msg.sender and flawed BalanceTracker logic if the code were reachable.

test/enforcers/ERC20TotalBalanceChangeEnforcer.t.sol#L376-L380

token.transfer(delegator, 21);
vm.prank(dm);
vm.expectRevert("ERC20TotalBalanceChangeEnforcer:exceeded-balance-decrease");
enforcer.afterAllHook(terms_, hex"", singleDefaultMode, mintExecutionCallData, bytes32(0), delegator, delegate);
enforcer.afterAllHook(terms_, hex"", singleDefaultMode, mintExecutionCallData, bytes32(0), delegator, delegate);

test/enforcers/ERC20TotalBalanceChangeEnforcer.t.sol#L442-L445

vm.prank(dm);
vm.expectRevert("ERC20TotalBalanceChangeEnforcer:insufficient-balance-increase");
enforcer.afterAllHook(termsIncrease_, hex"", singleDefaultMode, mintExecutionCallData, bytes32(0), delegator, delegate);
enforcer.afterAllHook(termsDecrease_, hex"", singleDefaultMode, mintExecutionCallData, bytes32(0), delegator, delegate);

test/enforcers/ERC20TotalBalanceChangeEnforcer.t.sol#L467-L470

vm.prank(dm);
vm.expectRevert("ERC20TotalBalanceChangeEnforcer:exceeded-balance-decrease");
enforcer.afterAllHook(termsIncrease_, hex"", singleDefaultMode, mintExecutionCallData, bytes32(0), delegator, delegate);
enforcer.afterAllHook(termsDecrease_, hex"", singleDefaultMode, mintExecutionCallData, bytes32(0), delegator, delegate);

Fix in CursorFix in Web


Was this report helpful? Give feedback by reacting with 👍 or 👎

@hanzel98
Copy link
Copy Markdown
Contributor Author

I can't approve it because I created the PR, but it looks good to me. Thanks @MoMannn

@McOso McOso merged commit 3ddf4f7 into main Jul 25, 2025
9 checks passed
@McOso McOso deleted the chore/balance-payment-enforcement branch July 25, 2025 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new release This PR goes to the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants