Skip to content

Fix double-counting in getVTokenPremium1155.t.sol#109

Open
1d34h4z4rd wants to merge 1 commit intoNFTX-project:masterfrom
1d34h4z4rd:master
Open

Fix double-counting in getVTokenPremium1155.t.sol#109
1d34h4z4rd wants to merge 1 commit intoNFTX-project:masterfrom
1d34h4z4rd:master

Conversation

@1d34h4z4rd
Copy link
Copy Markdown

forge test output:

Ran 41 test suites in 1.84s (12.02s CPU time): 285 tests passed, 0 failed, 0 skipped (285 total tests)


Fix incorrect inventory tracking in fuzz test for getVTokenPremium1155

Summary

This PR fixes a logic bug in the fuzz test
test_WhenTheAmountIsLessThanOrEqualToTheQuantityOfNftsInTheVault.

The test previously double-counted deposited NFTs, which caused incorrect assumptions about the vault's inventory and resulted in unexpected NFTInventoryExceeded() reverts during fuzzing.

This was a test-side accounting error, not a protocol vulnerability.


Root Cause

Inside the deposit loop, the test incremented totalDeposited twice:

totalDeposited += depositAmounts[i];          // first increment
...
vault.mint(...);
totalDeposited += depositAmounts[i];          // second increment

Since the vault only receives one deposit per iteration, the test inflated the perceived inventory to 2 × actual.

As a result, fuzzing could pick values where:

  • Real vault inventory = N
  • Test’s totalDeposited = 2N
  • Fuzzer chooses an amount A where N < A ≤ 2N

This made the test incorrectly expect success, while the contract correctly reverted.


Fix

The double-counting has been removed. Now each deposit is counted exactly once:

vault.mint({...});
totalDeposited += depositAmounts[i];

This aligns totalDeposited with the vault’s actual internal accounting.


Impact

  • Eliminates false positives from fuzzing.
  • Ensures test assumptions match protocol behavior.
  • Increases correctness and stability of the test suite.
  • No production or contract-level changes.

Testing

  • Fuzz tests now pass consistently.
  • No new failures introduced.
  • Behavior now matches the expected premium logic for ERC1155 vaults.

@1d34h4z4rd
Copy link
Copy Markdown
Author

CC @apoorvlathey @alexgausman

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