Approved workflow

testReentrantTokenCannotWithdrawTwice is the only test covering the approved requirement 'reentrancy through a malicious token'. It asserts only that the nested withdraw(target) call fails. That nested call fails because of checks-effects-interactions (withdraw sets userLock.amount = 0 before calling token.transfer, so the reentrant call hits LockNotFound), not because of the nonReentrant guard.

The guard on withdraw therefore has zero effective coverage: it can be removed without any test noticing, and a later refactor that moves the token.transfer above the state write would silently remove the double-withdraw protection while this suite stays green. A test that pins the guard must assert the reentrant call reverts with TimeLockSavings.ReentrantCall specifically, or re-enter a different still-locked lock id, which CEI alone does not block.

No test re-enters createLock. The shipped code is safe (I verified the nested call reverts), but the safety comes entirely from the nonReentrant modifier, and nothing pins it.

This matters more than the withdraw case because createLock's deposit accounting is not self-protecting: balanceBefore is sampled at src/TimeLockSavings.sol:63 and compared at :65-:68 across an external token.transferFrom call, so with the guard absent a nested createLock executing inside transferFrom makes the outer call observe a balance delta that includes the inner deposit, and the outer balanceAfter - balanceBefore != amount check then either reverts a legitimate deposit or, for the inner call, accepts a delta it did not fund.

The requirement 'reentrancy through a malicious token' is not met for the deposit path.

testRejectsFalseReturningToken (test/TimeLockSavings.t.sol:133) only covers the createLock path: FalseToken makes transferFrom return false, so the test never reaches withdraw. Neither Timebox nor ReentrantToken ever returns false from transfer, so the return-value check on the withdraw path is never exercised.

The consequence if that check is ever dropped is worse than on the deposit path, because withdraw has already set userLock.amount = 0 and decremented totalLocked before the transfer: the lock would be destroyed and LockWithdrawn emitted while the depositor receives nothing, with no recovery path (the contract has no rescue function by design).

README.md:15-17 states as a contract property that 'a deposit is accepted only when the contract's token balance increases by exactly the requested amount, excluding fee-on-transfer or otherwise incompatible tokens'. No test constructs a token whose balance delta differs from the requested amount, so this documented property is unverified.

TBOX itself is not fee-on-transfer, so this is a coverage and documentation-accuracy gap rather than a live vulnerability, but the claim is currently unbacked.

createLock stores unlockTime in a uint64 (src/TimeLockSavings.sol:16, :71) and guards the narrowing cast at line 61. No test covers either the accepted edge or the rejected one, even though the acceptance criteria call for timing boundaries.

The guard matters: without it the cast silently truncates, and the specific value 2**64 truncates to 0, producing a lock whose unlockTime is in the past and which the depositor can withdraw in the very next transaction - the exact 'nothing can be withdrawn early' invariant the protocol exists to enforce.

The constructor rejects address(0) and EOAs/undeployed addresses, and this is the single piece of deployment-time validation that stands between the manifest's $token substitution and a permanently bricked vault. No test asserts it. This is the argument the manifest fills (launch.json contracts[0].constructorArgs == ["$token"]), so it is the one constructor input a reviewer of the manifest/source pair would want pinned by a test.

transferFrom special-cases permitted == type(uint256).max to skip the allowance decrement. test/Timebox.t.sol:21-28 only exercises a finite allowance of 40. test/TimeLockSavings.t.sol:80 approves type(uint256).max but never asserts the allowance afterwards, so the branch is executed and never observed.

The approved website flow ('connect, approve TBOX, create a lock') will use an unlimited approval, so a regression here changes user-visible behaviour (a one-shot approval silently becoming consumed) with no test failing.

The approved brief requires 'events for every state change', and README.md:24 states LockCreated and LockWithdrawn cover every persistent state transition. test/TestBase.sol declares only prank, warp and expectRevert (test/TestBase.sol:4-9) - there is no expectEmit and no recordLogs - so no test observes any log.

The events are the interface the next stage depends on: the website and any indexer read lock amounts and unlock times from them for the 'list my locks with countdowns' view, and an incorrect payload would not be caught anywhere in this repository. Separately, LockWithdrawn is emitted after the external token.transfer (src/TimeLockSavings.sol:86-87) rather than with the state change; harmless for TBOX, but it means the log ordering is not part of any tested guarantee either.

The 'Build and test' section documents only forge build / forge test / forge fmt --check. The supplied protected suites are driven entirely by environment variables, and a reader following the README has no way to run them or to know why they skip.

Token.protected.t.sol needs IMD_TOKEN_CREATION_CODE and IMD_TOKEN_DECIMALS; Project.protected.t.sol needs IMD_PROJECT_FACTORY, IMD_PROJECT_CHAIN_ID, IMD_EXPECTED_TOKEN, IMD_EXPECTED_SUPPLY, IMD_PROJECT_COUNT and, per project index i, IMD_PROJECT_CODE_i, IMD_PROJECT_SALT_i and IMD_PROJECT_ADDRESS_i. Plain forge test correctly does not depend on any of them, which is the right default; only the documentation is missing.

Withdrawal zeroes userLock.amount (src/TimeLockSavings.sol:84) but leaves the id in _lockIds, and nothing removes it.

This is the documented design (README.md:22-23) and is not a defect in the contract, but it is the enumeration contract the next stage inherits: the 'list my locks with countdowns' view must treat amount == 0 as withdrawn rather than as an open lock, and the returned array grows without bound for an address that keeps creating locks, so a client that eth_calls lockIdsOf for a heavy user will eventually hit a node return-size or gas cap.

Recording it here so the frontend assignment does not rediscover it against live data.