Add audit scorecard for Rajat staking submission#1
Conversation
RolandDivine
deleted the
codex/audit-rajat-s-staking-contract-submission-wljbt5
branch
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9332cceb48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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. |
There was a problem hiding this comment.
requestWithdrawal in the truncation finding
The narrowing-cast section currently describes this as a large-token-amount problem, but requestWithdrawal() is exploitable with no large balances at all. In src/StakingPool.sol:301-325, both the sufficiency check and the stored request amount use uint128(amount), so calling requestWithdrawal(2**128) compares against zero and creates a zero-amount request even for an unstaked account, incrementing nextWithdrawalId and growing _userWithdrawalIds. Because the scorecard omits this path, it materially understates the reachability/severity of the accounting bug and makes the overall “Pass” recommendation too lenient.
Useful? React with 👍 / 👎.
| ### 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. |
There was a problem hiding this comment.
Requirement 6 is graded as a “Mostly pass” because the Yul helpers supposedly provide overflow handling, but the withdrawal-path helper _add128 does not actually enforce a 128-bit upper bound. In src/StakingPool.sol:655-664, if lt(result, a) only detects 256-bit wraparound; type(uint128).max + 1 produces 2**128, which is then truncated when returned to Solidity. Since requestWithdrawal() relies on this helper for totalPendingWithdrawals, the submission does not meet the claimed overflow-handling bar in the withdrawal math itself.
Useful? React with 👍 / 👎.
1 participant
Motivation
Description
AUDIT.mdcontaining a structured scorecard (79/100), a requirement-by-requirement assessment, a list of strengths, and prioritized issues with remediation suggestions.uint256 -> uint128narrowing casts, checkpoint-based oracle semantics vs per-block historical rates, and absence of a verified 100% coverage artifact).AUDIT.mdand include the review rationale and commands used during the review (file inspections and grep/reading of tests and sources).Testing
forge test -vv,forge coverage --report summary, andforge snapshot, but each command failed because theforgebinary is not available in the execution environment, so unit/fuzz/invariant test runs could not be executed here.rg,sed,nl, andcatto enumerate and read relevant files (src/StakingPool.sol,test/StakingPool.t.sol,test/invariants/*,thoughts.md,GAS_REPORT.md) to validate claims and reproduce the reasoning in the audit.AUDIT.mdfile to the repo (commit message: "Add audit scorecard for staking submission").Codex Task