Add TWAP price-oracle and cross-function reentrancy check classes#62
Open
Pattermesh wants to merge 1 commit into
Open
Add TWAP price-oracle and cross-function reentrancy check classes#62Pattermesh wants to merge 1 commit into
Pattermesh wants to merge 1 commit into
Conversation
Adds the two highest-frequency real-world exploit classes that were missing from the executable checklist, each with vulnerable + safe example contracts and self-checking Foundry test templates that pass against the safe variant and catch the vulnerable variant. TWAPOracleCheck: models the defense, not just the price diff. Manipulates the underlying source within a single block and asserts the price the contract acts on resists it. A spot reader moves and is flagged; a TWAP reader is unmoved and passes. (bZx / Harvest / Inverse Finance class.) CrossFunctionReentrancyCheck: two functions share state, one calls out before settling, the attacker re-enters through the OTHER function to double-spend. A nonReentrant guard on only one path does not stop it. (Lendf.Me / dForce class, 2020.) Both checks expose a side-effect-free detector returning bool (so harnesses can assert detection without tripping the global failure flag) plus the conventional test_* method that calls fail() on detection. Wired into the README table + architecture, the web explorer, and the CI gate exclusion list, mirroring the existing checks. Verification: - forge build: Compiler run successful (new files warning-clean) - forge test --no-match-contract '^(Example|TestERC4626)': 9 passed, 0 failed (incl. both new safe-variant suites) - Detection demos: TWAP flags 1150% single-block drift; cross-function flags 1e18 double-spend credit parked on accomplice. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+60
to
+68
| function withdraw() external { | ||
| uint256 bal = balances[msg.sender]; | ||
| require(bal > 0, "no balance"); | ||
|
|
||
| (bool sent,) = msg.sender.call{value: bal}(""); | ||
| require(sent, "transfer failed"); | ||
|
|
||
| balances[msg.sender] = 0; // Too late — credit already moved cross-function. | ||
| } |
Comment on lines
+102
to
+128
| contract CrossFunctionReentrantAttacker { | ||
| address public target; | ||
| bytes public withdrawPayload; | ||
| bytes public reentrantPayload; | ||
| bool private entered; | ||
|
|
||
| constructor(address _target, bytes memory _withdraw, bytes memory _reentrant) { | ||
| target = _target; | ||
| withdrawPayload = _withdraw; | ||
| reentrantPayload = _reentrant; | ||
| } | ||
|
|
||
| function attack() external { | ||
| (bool ok,) = target.call(withdrawPayload); | ||
| ok; // detection is via surviving credit, not call success | ||
| } | ||
|
|
||
| receive() external payable { | ||
| if (!entered) { | ||
| entered = true; | ||
| // Cross-function re-entry: call the OTHER function while the | ||
| // withdraw that sent us this ETH has not yet zeroed our balance. | ||
| (bool ok,) = target.call(reentrantPayload); | ||
| ok; | ||
| } | ||
| } | ||
| } |
|
|
||
| /// @dev Attacker that re-enters a DIFFERENT function on the payout callback. | ||
| contract CrossFunctionReentrantAttacker { | ||
| address public target; |
Comment on lines
+39
to
+69
| contract VulnerableCrossFunctionVault { | ||
| mapping(address => uint256) public balances; | ||
|
|
||
| function deposit() external payable { | ||
| balances[msg.sender] += msg.value; | ||
| } | ||
|
|
||
| /// @dev Internal transfer of credit between accounts. Shares `balances` | ||
| /// with withdraw() but has NO reentrancy protection and can be called | ||
| /// mid-withdraw. | ||
| function transfer(address to, uint256 amount) external { | ||
| require(balances[msg.sender] >= amount, "insufficient"); | ||
| balances[msg.sender] -= amount; | ||
| balances[to] += amount; | ||
| } | ||
|
|
||
| /// @dev BUG: external call before the state write. While the call is in | ||
| /// flight `balances[msg.sender]` is still funded, so a re-entrant | ||
| /// `transfer()` can siphon that credit to an accomplice. The later | ||
| /// `balances[msg.sender] = 0` zeroes an already-emptied slot — the | ||
| /// moved credit survives. | ||
| function withdraw() external { | ||
| uint256 bal = balances[msg.sender]; | ||
| require(bal > 0, "no balance"); | ||
|
|
||
| (bool sent,) = msg.sender.call{value: bal}(""); | ||
| require(sent, "transfer failed"); | ||
|
|
||
| balances[msg.sender] = 0; // Too late — credit already moved cross-function. | ||
| } | ||
| } |
Comment on lines
+75
to
+98
| contract SafeCrossFunctionVault is SimpleReentrancyGuard { | ||
| mapping(address => uint256) public balances; | ||
|
|
||
| function deposit() external payable { | ||
| balances[msg.sender] += msg.value; | ||
| } | ||
|
|
||
| function transfer(address to, uint256 amount) external nonReentrant { | ||
| require(balances[msg.sender] >= amount, "insufficient"); | ||
| balances[msg.sender] -= amount; | ||
| balances[to] += amount; | ||
| } | ||
|
|
||
| function withdraw() external nonReentrant { | ||
| uint256 bal = balances[msg.sender]; | ||
| require(bal > 0, "no balance"); | ||
|
|
||
| // Checks-effects-interactions AND a shared lock — belt and suspenders. | ||
| balances[msg.sender] = 0; | ||
|
|
||
| (bool sent,) = msg.sender.call{value: bal}(""); | ||
| require(sent, "transfer failed"); | ||
| } | ||
| } |
Comment on lines
+48
to
+56
| function _accumulate() internal { | ||
| uint32 nowTs = uint32(block.timestamp); | ||
| uint32 timeElapsed = nowTs - blockTimestampLast; | ||
| if (timeElapsed > 0 && reserve0 > 0) { | ||
| uint256 price0 = (uint256(reserve1) * 1e18) / uint256(reserve0); | ||
| price0CumulativeLast += price0 * timeElapsed; | ||
| blockTimestampLast = nowTs; | ||
| } | ||
| } |
Contributor
|
Welcome to kcolbchain, @Pattermesh — glad you're here. 🌱 Here's what happens from this PR:
While you wait:
What happens after your first merge
Thanks for writing the code. We're building this to last. |
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
3 participants
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.
What
Adds the two highest-frequency real-world exploit classes that were missing from the executable checklist, each wired in exactly like the existing checks (abstract check class subclassing
ChecklistBase+ vulnerable/safe example contracts + Foundry test template + README/web/CI wiring):1.
TWAPOracleCheck— price-oracle / TWAP manipulationThe stronger sibling of
OracleCheck: instead of merely diffing a price before/after, it models the defense. It manipulates the underlying source within a single block (no time advance) and asserts that the price the contract under audit acts on does not drift past a tolerance (default 10%).VulnerableSpotOracleConsumer) jumps immediately and is flagged.SafeTWAPOracleConsumer) is unmoved within one block — its accumulator only advances with elapsed time — and passes.MockTWAPPairexposing both a spot price and a cumulative-price accumulator.2.
CrossFunctionReentrancyCheck— cross-function reentrancyDistinct from
ReentrancyCheck(single-function CEI re-entry). Here two functions share state, one (withdraw) makes an external call before settling that state, and the attacker re-enters through the other function (transfer) during the callback to move its still-credited balance to an accomplice — turning one deposit into a double-spend.VulnerableCrossFunctionVault(unguarded sharedbalances) is flagged.SafeCrossFunctionVault(sharednonReentrantlock spanning both entry points) passes.Both checks expose a side-effect-free
detect…()returningbool(so harnesses can assert detection without tripping forge-std's global failure flag) plus the conventionaltest_*method that callsfail()on detection, matching every other check in the repo.Wiring
README.md— vulnerability-class table + architecture treeweb/index.html— two new entries in the interactive explorer (render now honors an optional per-checkruncommand).github/workflows/audit.yml— added the two newExample…Auditdemo contracts to the documented gate-exclusion list, consistent with the existing demosVerification
The two new safe-variant suites (
TWAPOracleCheckPassesTWAPConsumer,CrossFunctionReentrancyCheckPassesSafe) pass in the clean CI gate. Detection demos confirm the vulnerable variants are caught:🤖 Generated with Claude Code