Test suite
When building a complex contract ecosystem with multiple moving parts and dependencies, comprehensive testing is essential. This includes testing for both positive and negative scenarios. Positive tests should verify that each function's side effect is as expected, while negative tests should cover every revert, preferably in every logical branch.
However, several functions in the smart contracts are not covered by any unit or integration tests, to the best of our knowledge. The Forge tests cover most of the functions within the scope of this audit. However, the following functions and require statements have no test coverage.
PDPVerifier.sol
getProofSetListener
No test case covers this function.
addRoots
There is no test case for the scenario where the length of the provided
extraData
exceedsEXTRA_DATA_MAX_SIZE
.There is no test case ensuring that if someone other than the owner of the proof set tries to call this function, it will revert.
scheduleRemovals
There is no test case for the scenario where the length of the provided
extraData
exceedsEXTRA_DATA_MAX_SIZE
.There is no test case for the scenario where the proof set is not valid.
There is no test case for the scenario where the current length of
scheduledRemovals
, after adding newrootIds
, exceedsMAX_ENQUEUED_REMOVALS
.
provePossession
There is no test case ensuring that if someone other than the owner of the proof set tries to call this function, it will revert.
There is no test case for the scenario where
challengeEpoch
is not scheduled.
nextProvingPeriod
There is no test case for the scenario where the length of the provided
extraData
exceedsEXTRA_DATA_MAX_SIZE
.There is no test case ensuring that if someone other than the owner of the proof set tries to call this function, it will revert.
There is no test case for the scenario where the provided
challengeEpoch
is less thanblock.number + challengeFinality
.
Cids.sol
digestFromCid
There is no test case for the scenario where the length of the provided
cid.data
is less than32
.
MerkleVerify.sol
verifyCalldata
No test case covers this function.
processProofCalldata
No test case covers this function.
SimplePDPService.sol
possessionProven
There is no test case for the scenario where the
provingDeadlines[proofSetId]
is not set yet.
The test coverage for this project should be expanded to include all contracts, functions, and test cases provided above, not just surface-level functions.
Good test coverage has multiple effects.
It finds bugs and design flaws early (preaudit or prerelease).
It gives insight into areas for optimization (e.g., gas cost).
It displays code maturity.
It bolsters customer trust in your product.
It improves understanding of how the code functions, integrates, and operates — for developers and auditors alike.
It increases development velocity long-term.
The last point seems contradictory, given the time investment to create and maintain tests. To expand upon that, tests help developers trust their own changes. It is difficult to know if a code refactor — or even just a small one-line fix — breaks something if there are no tests. This is especially true for new developers or those returning to the code after a prolonged absence. Tests have your back here. They are an indicator that the existing functionality most likely was not broken by your change to the code.
Remediation
This issue has been acknowledged by Filecoin, and fixes were implemented in the following commits: