Skip to content

refactor: implement pull-over-push pattern for fees#2

Open
lofingv wants to merge 3 commits intoOpenGradient:mainfrom
lofingv:fix/pull-over-push-pattern
Open

refactor: implement pull-over-push pattern for fees#2
lofingv wants to merge 3 commits intoOpenGradient:mainfrom
lofingv:fix/pull-over-push-pattern

Conversation

@lofingv
Copy link

@lofingv lofingv commented Feb 4, 2026

Overview

This PR introduces a major security improvement by implementing the Pull-over-Push pattern for subject fee distribution.

Why this is necessary

Currently, buyShares and sellShares attempt to send ETH/BNB directly to the digitalTwinOwner.
If an owner is a smart contract that rejects incoming transfers (no receive function), it creates a Denial of Service (DoS). No one would be able to buy or sell shares of that specific twin.

Changes

  • Added claimableFees mapping to store fees securely.
  • Updated buyShares and sellShares to accrue fees instead of pushing them.
  • Added a withdrawFees() function for owners to claim their earnings.

This ensures the core trading logic remains functional regardless of the owner's contract implementation.

@lofingv
Copy link
Author

lofingv commented Feb 4, 2026

Updated the PR to include a gas optimization for the getPrice function. I've wrapped the sum-of-squares calculation in an unchecked block, as the supply and amount values in this context won't trigger an overflow. This will reduce transaction costs for users.

@lofingv
Copy link
Author

lofingv commented Feb 5, 2026

Summary of Changes

I have completed the update of the test suite to ensure full compatibility with the Pull-over-Push fee distribution pattern. The previous tests were failing because they expected immediate ETH transfers, which is exactly the vulnerability my PR addresses.

Key Improvements:

  • Refactored Security Tests: Updated testBuySharesDistributesFees and testSellSharesDistributesFees to verify that fees are correctly accounted for in the claimableFees mapping instead of checking direct balance increases.
  • DoS Protection Verified: Updated testRevertingTwinOwnerDoesNotBlockBuying and similar cases. These tests now prove that a malicious or broken fee recipient can no longer block the entire trading logic.
  • New Test Coverage: Added testWithdrawFees to ensure that authors can successfully claim their accumulated fees and that the balance resets correctly after withdrawal.
  • Code Quality: Fixed logic flow and contract structures in the test file to ensure all helper contracts are correctly scoped.

The codebase is now more resilient against DoS attacks and follows the best practices for smart contract fee management. Ready for final review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant