Skip to content
This repository was archived by the owner on May 6, 2026. It is now read-only.

AUDIT.md

Latest commit

 

History

History
135 lines (91 loc) · 7.57 KB

AUDIT.md

File metadata and controls

135 lines (91 loc) · 7.57 KB
Raw

Audit Scorecard for Rajat's Staking Submission

Verdict

Pass, but not a clean pass.

The submission demonstrates solid Solidity fundamentals and implements most of the requested functionality:

  • Foundry project structure is present.
  • The staking pool supports ERC-20 LP staking plus ERC-1363 callback-based staking.
  • The 2-day cooldown + multisig reset flow is implemented.
  • Re-entrancy is mitigated with CEI plus a custom mutex.
  • The code includes Yul assembly for hot-path math.
  • There is a design write-up and a gas report.

However, there are meaningful gaps against the prompt and at least one real correctness issue:

  • The reward-rate model does not truly preserve per-block historical rates; it lazily applies the latest oracle rate to the entire unchecked interval.
  • Several uint256 -> uint128 casts happen without bounds checks, which can truncate values and desynchronize accounting for large inputs.
  • The repository does not prove the claimed 100% test coverage.
  • The required deliverable name is THOUGHTS.md, but the repo contains thoughts.md.

Score

79 / 100

Breakdown

Category Max Score Notes
Project setup / Foundry deliverable 10 10 Foundry project is present and configured.
Core staking + cooldown logic 25 22 Main flow is implemented well.
Security / re-entrancy design 20 17 Good CEI + mutex approach, but accounting truncation is a real issue.
Oracle-driven dynamic rewards 15 9 Oracle exists, but rate changes are not faithfully accounted on a true per-block basis.
ERC-1363 + assembly requirement 10 10 Both requirements are implemented.
Tests / fuzz / invariants 15 7 Strong test effort, but no proof of 100% coverage and some features appear untested.
Gas report + design write-up 5 4 Both are present, but gas numbers were not reproducible in this environment and thoughts.md is misnamed.

What was done well

  1. The requested staking lifecycle is present. The contract supports stake, request withdrawal, execute withdrawal after cooldown, reward claiming, and multisig flagging. This maps well to the scenario in the prompt.

  2. The cooldown / multisig mechanism is implemented correctly at a high level. requestWithdrawal() creates a pending request, executeWithdrawal() enforces a 2-day wait, and flagWithdrawal() resets the timer by overwriting requestTime. The multisig can delay but not seize funds.

  3. Re-entrancy mitigation is stronger than a default OZ-only solution. The contract uses both CEI and a custom _locked mutex. The write-up also explains why both layers exist.

  4. ERC-1363 support is real, not superficial. The token side implements transferAndCall, transferFromAndCall, and approveAndCall, and the pool implements onTransferReceived with an LP-token-only gate.

  5. The code shows conscious gas work. Packed storage, custom errors, a merged reward-settlement path, and Yul math all point to deliberate optimization.

Main issues

1. High severity: unchecked uint256 -> uint128 truncation can corrupt accounting

The contract stores critical balances as uint128, but several external-entry functions accept uint256 and cast to uint128 without validating the upper bound first. Examples include reward funding and staking. In Solidity, narrowing a uint256 to uint128 does not revert; it truncates the high bits.

Affected examples:

  • fundRewards() does rewardReserve += uint128(amount) inside unchecked.
  • stake() does u.stakedAmount += uint128(amount) and totalStaked += uint128(amount).
  • onTransferReceived() does the same for ERC-1363 staking.

Impact: if a caller supplies an amount above type(uint128).max, the token transfer can move the full amount while the pool only records the truncated lower 128 bits. That breaks reserve/accounting invariants and can strand or misaccount funds.

2. Medium severity: reward oracle model is not truly “reward rate changes every block” accounting

The pool updates rewards lazily in _updatePool(): it reads the oracle once at checkpoint time and applies that single rate to all blocks since lastRewardBlock.

The test suite explicitly documents this behavior: if the rate changes mid-window without an intermediate checkpoint, the new rate is retroactively applied to the full uncheckpointed period. That is a workable simplification, but it is not the literal behavior described in the prompt.

Impact: users are rewarded based on checkpoint timing, not a faithful per-block historical rate schedule. This is a design mismatch with the stated requirement.

3. Medium severity: the “100% test coverage” deliverable is not substantiated

The repo includes a substantial test suite plus invariants, which is good. But the repository does not include a coverage artifact proving 100% coverage, and some code paths appear untested from static inspection:

  • MockRewardOracle.setVariableRate() exists but is not referenced by the tests.
  • ERC-1363 transferFromAndCall and approveAndCall are implemented, but the visible tests focus on transferAndCall only.

Impact: this falls short of the explicit deliverable requirement unless a real coverage report is produced.

4. Low severity: deliverable file name mismatch

The prompt requested THOUGHTS.md, while the repo contains thoughts.md.

Impact: minor, but still a deliverable miss.

Requirement-by-requirement assessment

1) Hardhat/Foundry project

Pass. The project is clearly a Foundry repo with fuzz and invariant settings in foundry.toml.

2) Users stake ERC-20 LP tokens

Pass. Standard approve + stake flow is implemented, and ERC-1363 adds a one-transaction path.

3) Rewards accumulate from an oracle-driven rate

Partial pass. The oracle interface and mock exist, but the implementation is checkpoint-based rather than faithfully historical per block.

4) 2-day timelocked withdrawal

Pass. Cooldown enforcement is implemented and tested.

5) Security multisig can flag suspicious withdrawals and reset cooldown

Pass. Implemented as specified.

6) Assembly/Yul used in withdrawal math with overflow handling

Mostly pass. Yul math helpers exist and have explicit overflow checks. But the contract still has unrelated narrowing-cast truncation risk elsewhere, so the overall overflow story is not perfect.

7) ERC-1363 support

Pass. Present in both token and pool integration.

8) 100% test coverage including fuzzing/invariant tests

Fail / not demonstrated. Fuzzing and invariants exist, but 100% coverage is not proven and static inspection suggests uncovered paths.

9) Gas report

Pass with reservation. A gas report file is present, but I could not reproduce it in this environment because Foundry is not installed.

10) THOUGHTS.md explaining data structures and re-entrancy mitigation beyond OZ modifiers

Partial pass. The content is good, but the file is named thoughts.md instead of THOUGHTS.md.

Final recommendation

If this were a hiring exercise, I would mark this as passing but with follow-up concerns.

Rajat showed:

  • good protocol architecture,
  • strong Solidity fluency,
  • awareness of gas/storage tradeoffs,
  • and a reasonable security mindset.

But I would absolutely ask for a revision before calling it production-ready:

  1. add upper-bound checks before every uint128 cast,
  2. either redesign reward accounting for historical per-block rates or explicitly redefine the oracle requirement,
  3. produce a real coverage report proving 100% if that claim is kept,
  4. rename thoughts.md to THOUGHTS.md.