Skip to content

Commit 019b21e

Browse files
kupermindkupermind
authored
Merge pull request #15 from LemonTreeTechnologies/post-audit
refactor: addressing audit 8 and 9
2 parents 57d4094 + f1590d2 commit 019b21e

13 files changed

Lines changed: 414 additions & 96 deletions

‎CLAUDE.md‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
# CLAUDE.md
2+
3+
This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository.
4+
5+
## Project Overview
6+
7+
stOLAS is an ERC4626 liquid staking vault for OLAS tokens. Users deposit OLAS on L1 (Ethereum) and receive stOLAS tokens. The deposited OLAS is bridged to L2 chains (Gnosis, Base, Mode) for active staking in the Autonolas service ecosystem. Rewards flow back from L2 to L1, increasing the stOLAS price-per-share.
8+
9+
## Build & Test Commands
10+
11+
```bash
12+
make install # Install all dependencies (poetry, forge, yarn)
13+
make build # forge build
14+
make fmt # forge fmt && forge fmt --check
15+
make lint # solhint on contracts/**/*.sol
16+
make tests # forge test -vvv
17+
make tests-hardhat # Hardhat JS tests (temporarily renames LzOracle.sol)
18+
make tests-coverage # forge coverage -vvvv
19+
20+
# Run a single Forge test file
21+
forge test --match-path "test/LiquidStaking.t.sol" -vv
22+
23+
# Run a single Forge test function
24+
forge test --match-test "testDepositAndStake" -vvv
25+
26+
# Hardhat test variants
27+
npm run test:hardhat # Full JS test suite
28+
npm run test:fast # Optimized subset
29+
npm run test:original # Original comprehensive suite
30+
```
31+
32+
**Note:** Hardhat tests temporarily rename `LzOracle.sol` to avoid compilation conflicts. The npm scripts handle this automatically.
33+
34+
## Solidity Configuration
35+
36+
- **Compiler:** 0.8.30, optimizer enabled (1M runs), viaIR, EVM target: Prague
37+
- **Source directory:** `contracts/`
38+
- **Key remappings** (in `foundry.toml`): `@openzeppelin`, `@solmate`, `@registries`, `@layerzerolabs`, `@gnosis.pm`
39+
40+
## Architecture
41+
42+
### L1 Contracts (`contracts/l1/`)
43+
44+
| Contract | Role |
45+
|---|---|
46+
| **stOLAS** | ERC4626 vault. Tracks `stakedBalance`, `vaultBalance`, `reserveBalance`. PPS = totalReserves / totalSupply |
47+
| **Depository** | Routes deposits to L2 staking models. Manages model lifecycle (Active/Inactive/Retired) and product types (Alpha/Beta/Final) |
48+
| **Treasury** | Withdrawal requests via ERC6909 tokens. Triggers L2 unstaking if vault liquidity is insufficient |
49+
| **Distributor** | Receives bridged rewards. Splits between veOLAS lock and stOLAS vault top-up |
50+
| **Lock** | veOLAS management for governance voting power |
51+
| **UnstakeRelayer** | Receives OLAS from retired L2 staking models, returns to stOLAS reserve |
52+
53+
### L2 Contracts (`contracts/l2/`)
54+
55+
| Contract | Role |
56+
|---|---|
57+
| **StakingManager** | Orchestrates service deployment/staking. Creates ActivityModule BeaconProxy instances per service |
58+
| **ExternalStakingDistributor** | Manages external staking on third-party staking proxies. Deploys services (creates Safe multisigs with self as module), stakes/unstakes/re-stakes them, claims and distributes rewards (split between Collector, protocol, and curating agent per configurable factors). Supports V1 (rewards on multisig) and V2 (rewards on contract) staking types. Access-controlled via owner, whitelisted managing agents (for unstakes), and per-proxy curating agents guarded by staking guards. Receives OLAS deposits from L2 staking processor and handles withdraw/unstake requests back through Collector |
59+
| **StakingTokenLocked** | Restricted StakingToken that only allows StakingManager as staker |
60+
| **ActivityModule** | Per-service BeaconProxy. Verifies liveness, shields staking funds, triggers reward claims |
61+
| **Collector** | Collects L2 rewards and bridges to L1 (REWARD→Distributor, UNSTAKE→Treasury, UNSTAKE_RETIRED→UnstakeRelayer) |
62+
63+
### Bridging (`contracts/l1/bridging/`, `contracts/l2/bridging/`)
64+
65+
- LayerZero V2 for cross-chain messaging
66+
- Chain-specific processors: `GnosisDepositProcessorL1`/`L2`, `BaseDepositProcessorL1`/`L2`, `DefaultDepositProcessorL1`/`L2`
67+
- `LzOracle` for LayerZero-driven staking model management
68+
69+
### Core Patterns
70+
71+
- **Proxy architecture:** UUPS-style via `Implementation.sol` (owner management + upgrade logic) and `Beacon.sol` (BeaconProxy for ActivityModules)
72+
- **ERC standards:** ERC4626 (vault), ERC6909 (withdrawal tickets), ERC721 (service NFTs from Autonolas registry)
73+
- **Libraries:** Solmate (ERC4626, ERC6909, ERC721), OpenZeppelin (security utilities), Autonolas Registries (staking infra)
74+
75+
### Cross-Chain Flow
76+
77+
```
78+
Stake: Depository (L1) → DepositProcessor → Bridge → StakingProcessorL2 → StakingManager (L2)
79+
Rewards: Collector (L2) → Bridge → Distributor (L1) → stOLAS
80+
Unstake: Collector (L2) → Bridge → Treasury/UnstakeRelayer (L1) → stOLAS
81+
```
82+
83+
## Deployment
84+
85+
- Scripts in `scripts/deployment/` follow numbered sequences: `deploy_l1_01_*` through `deploy_l1_15_*`, `deploy_l2_01_*` through `deploy_l2_09_*`
86+
- Configuration scripts: `script_l1_*`, `script_l2_*` for post-deployment setup
87+
- Contract addresses: `doc/configuration.json`
88+
- Finalized ABIs: `abis/0.8.30/`
89+
- Static audit: `./scripts/deployment/script_static_audit.sh eth_mainnet NETWORK_mainnet`
90+
91+
## ERC4626 Caveat
92+
93+
`deposit` is meant to be called only via **Depository**, and `redeem` only via **Treasury**. The `mint`/`withdraw` functions are non-standard and not for external use.
94+
95+
## Audit Findings & Resolutions
96+
97+
The project has undergone 9 internal audits (`audits/audit1` through `audits/audit9`) and 1 external audit (CODESPECT).
98+
99+
### audit8 (2026-03-08) — all informational, all fixed
100+
- **INFO-1**: Treasury `requestToWithdraw` didn't forward `msg.value` to unstake calls → Fixed: validates and forwards ETH correctly
101+
- **INFO-2**: Distributor `_increaseLock` left dangling OLAS approval on failure → Fixed: resets approval to 0
102+
- **INFO-3**: ERC6909 withdrawal tokens are transferable → By design
103+
- **INFO-4**: Permissionless trigger functions → By design
104+
105+
### audit9 (2026-03-18) — 5 Low, 5 Informational, all resolved
106+
- **L-1**: `reStake` used dead `mapCuratingAgents` mapping → Fixed: uses `mapServiceIdCuratingAgents[serviceId]`
107+
- **L-2**: `stOLAS.initialize()` no access control → By design (atomic deployment)
108+
- **L-3**: `DefaultDepositProcessorL1.drain()` sends ETH to `address(0)` → Fixed: removed `drain()` entirely (ETH cannot get stuck)
109+
- **L-4**: Distributor dangling approval → Fixed (same as audit8 INFO-2)
110+
- **L-5**: `StakingTokenLocked.maxNumInactivityPeriods` unused → By design (backward compatibility)
111+
- **INFO-1**: `setCuratingAgents` `stakingHash` in loop → Fixed: moved before loop
112+
- **INFO-2**: `unstakeAndWithdraw` missing entry validation → Fixed: added zero-checks at entry
113+
- **INFO-3**: `claim()` permissionless → By design
114+
- **INFO-4**: `unstakeRetired()` permissionless → By design
115+
- **INFO-5**: `LzOracle._lzReceive` trusts LZ Read → By design
116+
117+
## Modified Contracts (not yet re-deployed)
118+
119+
Contracts that have been modified but not yet re-deployed are tracked in
120+
[`MODIFIED_CONTRACTS.md`](MODIFIED_CONTRACTS.md). Keep that file in sync when changing a
121+
deployed contract, and clear entries once re-deployed.

‎MODIFIED_CONTRACTS.md‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
# Modified Contracts — Pending Re-Deployment
2+
3+
This file tracks contracts whose source has been **modified** after their last on-chain
4+
deployment but which have **not yet been re-deployed** ("modified but not updated").
5+
6+
Keep this list in sync whenever a deployed contract is changed, and clear an entry once
7+
the corresponding contract has been re-deployed and its address updated in
8+
`doc/configuration.json`.
9+
10+
## Pending audit8 / audit9 fixes
11+
12+
The following contracts were modified on the `post-audit` branch to address findings from
13+
internal audits 8 and 9. The fixes were reviewed and verified correct in
14+
[`audits/audit10/README.md`](audits/audit10/README.md) ("All 7 fixes verified correct.
15+
No new vulnerabilities introduced."). They still require re-deployment.
16+
17+
| Contract | Layer | Changes |
18+
|---|---|---|
19+
| `contracts/l1/Depository.sol` | L1 | `_unstake` refund now sent to `sender` (original caller) instead of `msg.sender` |
20+
| `contracts/l1/Treasury.sol` | L1 | `requestToWithdraw` now validates `msg.value`, forwards ETH to `unstakeExternal`/`unstake`; extracted `_processUnstakes` helper; added `WrongArrayLength` error |
21+
| `contracts/l1/Distributor.sol` | L1 | `_increaseLock` resets dangling OLAS approval to 0 in the zero-lock branch |
22+
| `contracts/l1/bridging/DefaultDepositProcessorL1.sol` | L1 | Removed `drain()` function and `Drained` event |
23+
| `contracts/l2/ExternalStakingDistributor.sol` | L2 | `unstakeAndWithdraw` moves staking-proxy reads inside the service-unstake condition and validates `stakingProxy`/`serviceId` at entry; `reStake` uses `mapServiceIdCuratingAgents` instead of dead `mapCuratingAgents`; `setCuratingAgents` computes `stakingHash` outside the loop |

‎README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,7 @@ The test suite covers comprehensive E2E scenarios:
155155
- **External Audits**: v0.1.0 code review is [completed](audits/README.md)
156156
- **Open Source**: Full transparency with public repository
157157
- **Bug Bounty**: Program under consideration post-audit
158+
- **Pending Re-Deployment**: Contracts modified but not yet re-deployed are tracked in [MODIFIED_CONTRACTS.md](MODIFIED_CONTRACTS.md)
158159

159160
## Roadmap
160161
> This roadmap reflects the current design and audit notes. Items and ordering may be updated by governance. *(Last updated: 2025-08-20 12:14 UTC)*

‎audits/README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ An `audit6` with a focus `main` branch [audit6](https://github.com/kupermind/ola
1111
An `audit7` with a focus `main` branch [audit7](https://github.com/LemonTreeTechnologies/olas-lst/blob/main/audits/audit7). <br>
1212
An `audit8` with a focus `main` branch [audit8](https://github.com/LemonTreeTechnologies/olas-lst/blob/main/audits/audit8). <br>
1313
An `audit9` — full project re-audit + delta review [audit9](https://github.com/LemonTreeTechnologies/olas-lst/blob/main/audits/audit9). <br>
14+
An `audit10` — post-audit fix review of `post-audit` branch [audit10](https://github.com/LemonTreeTechnologies/olas-lst/blob/main/audits/audit10). <br>
1415

1516
### External audits
1617
External audit reports are listed in their historical order:

‎audits/audit10/README.md‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
# Audit 10 — Post-Audit Fix Review
2+
3+
**Audit Date:** March 24, 2026
4+
**Commit Range:** `57d4094` (main) to `46f5737` (post-audit)
5+
**Branch:** `post-audit`
6+
**Repository:** `https://github.com/LemonTreeTechnologies/olas-lst`
7+
8+
## Objectives
9+
10+
Review of fixes addressing findings from internal audits 8 and 9, applied in the `post-audit` branch.
11+
Verify that all fixes are correct, complete, and do not introduce new vulnerabilities.
12+
13+
## Scope
14+
15+
5 modified contracts, 2 commits (`40a349c`, `46f5737`):
16+
17+
```
18+
contracts/l1/Depository.sol | 2 +-
19+
contracts/l1/Distributor.sol | 3 +
20+
contracts/l1/Treasury.sol | 109 ++++++++++++------
21+
contracts/l1/bridging/DefaultDepositProcessorL1.sol| 24 -
22+
contracts/l2/ExternalStakingDistributor.sol | 48 ++++-----
23+
```
24+
25+
## Findings
26+
27+
### Fix 1. Depository: refund sent to `msg.sender` instead of `sender`
28+
```
29+
File: contracts/l1/Depository.sol:324
30+
Before: (bool success,) = msg.sender.call{value: refund}("");
31+
After: (bool success,) = sender.call{value: refund}("");
32+
```
33+
The `sender` parameter is the original caller passed through from Treasury.
34+
When called via Treasury (which forwards `msg.sender` as `sender`),
35+
the refund was incorrectly going to Treasury (`msg.sender`) instead of
36+
the actual user (`sender`).
37+
38+
**Verdict: Fix correct. ✓**
39+
40+
### Fix 2. Distributor: dangling approval not reset
41+
```
42+
File: contracts/l1/Distributor.sol:89
43+
Added: IToken(olas).approve(lock, 0);
44+
```
45+
In the `else` branch (when lock amount is zero and no lock is created),
46+
a prior `approve(lock, olasAmount)` at line 79 leaves a non-zero allowance
47+
on the Lock contract. The fix resets approval to zero.
48+
49+
**Verdict: Fix correct. ✓**
50+
51+
### Fix 3. Treasury: ETH value not forwarded to bridge calls during unstake
52+
```
53+
File: contracts/l1/Treasury.sol:142-213
54+
Refactored into: _processUnstakes() internal function
55+
```
56+
The original `requestToWithdraw()` called `unstakeExternal()` and `unstake()`
57+
without forwarding `msg.value` for bridge gas payments. Bridge calls on Gnosis
58+
(AMB), Optimism (L1CrossDomainMessenger), etc. require native ETH for gas.
59+
ETH sent by the user would remain stuck in Treasury.
60+
61+
The fix:
62+
- Extracts `_processUnstakes()` helper (also resolves stack-too-deep)
63+
- Tracks `remainingValue` across external and LST unstake calls
64+
- Validates `externalAmounts.length == values[0].length`
65+
- Forwards `{value: externalValue}` to `unstakeExternal()` and `{value: remainingValue}` to `unstake()`
66+
- Rejects leftover ETH when no unstaking needed (`if (msg.value > 0) revert`)
67+
68+
**Verdict: Fix correct and complete. ✓**
69+
70+
### Fix 4. DefaultDepositProcessorL1: `drain()` function removed
71+
```
72+
File: contracts/l1/bridging/DefaultDepositProcessorL1.sol
73+
Removed: function drain() external { ... } (24 lines)
74+
```
75+
The `drain()` function allowed the owner to withdraw all native balance
76+
from the deposit processor. Removed to reduce attack surface — leftover
77+
refunds are handled via the existing `LeftoversRefunded` event path.
78+
79+
**Verdict: Fix correct. ✓**
80+
81+
### Fix 5a. ExternalStakingDistributor: guard checks moved inside service unstake condition
82+
```
83+
File: contracts/l2/ExternalStakingDistributor.sol:684-710
84+
```
85+
Previously, `availableRewards` and `stakingState` were read from `stakingProxy`
86+
even when no service unstake was requested (`stakingProxy == address(0)` or
87+
`serviceId == 0`). This caused reverts on invalid proxy addresses.
88+
89+
The fix moves all staking proxy reads inside the
90+
`if (stakingProxy != address(0) && serviceId > 0)` block.
91+
92+
**Verdict: Fix correct. ✓**
93+
94+
### Fix 5b. ExternalStakingDistributor: curating agent access control per-service
95+
```
96+
File: contracts/l2/ExternalStakingDistributor.sol:803
97+
Before: mapCuratingAgents[msg.sender]
98+
After: mapServiceIdCuratingAgents[serviceId] == msg.sender
99+
```
100+
Previously, any address in the generic `mapCuratingAgents` mapping could act
101+
on ANY service. The fix restricts to per-service curating agents via
102+
`mapServiceIdCuratingAgents[serviceId]`.
103+
104+
**Verdict: Fix correct. Addresses cross-service privilege escalation. ✓**
105+
106+
### Fix 5c. ExternalStakingDistributor: `stakingHash` computation moved outside loop
107+
```
108+
File: contracts/l2/ExternalStakingDistributor.sol:939
109+
```
110+
`keccak256(abi.encode(stakingGuard, stakingProxy))` was computed inside a loop
111+
traversing curating agents. Since the inputs don't change per iteration,
112+
this is a gas optimization only.
113+
114+
**Verdict: Fix correct (gas optimization). ✓**
115+
116+
## Summary
117+
118+
| # | Contract | Fix | Severity | Correct? |
119+
|---|----------|-----|:--------:|:--------:|
120+
| 1 | Depository | msg.sender → sender refund | Low | ✓ |
121+
| 2 | Distributor | Reset dangling approval | Low | ✓ |
122+
| 3 | Treasury | ETH value forwarding + validation | Medium | ✓ |
123+
| 4 | DefaultDepositProcessorL1 | Remove drain() | Low | ✓ |
124+
| 5a | ExternalStakingDistributor | Guards inside condition | Low | ✓ |
125+
| 5b | ExternalStakingDistributor | Per-service curating agent access | Medium | ✓ |
126+
| 5c | ExternalStakingDistributor | stakingHash outside loop | Gas | ✓ |
127+
128+
**All 7 fixes verified correct. No new vulnerabilities introduced.**

‎audits/audit8/README.md‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -88,33 +88,35 @@ The protocol demonstrates mature defensive coding patterns:
8888
### Medium: 0
8989
### Low: 0
9090

91-
### Informational: 4
91+
### Informational: 4 (all fixed)
9292

93-
#### INFO-1: Missing ETH Forwarding in Treasury.requestToWithdraw
93+
#### INFO-1: Missing ETH Forwarding in Treasury.requestToWithdraw — FIXED
9494
| | |
9595
|---|---|
9696
| **Location** | Treasury.sol:220-221, 227-228 |
9797
| **Description** | `requestToWithdraw` is `payable` but doesn't forward `msg.value` to `Depository.unstake`/`unstakeExternal` calls |
9898
| **Impact** | None currently — Optimism and Gnosis bridges don't require ETH for L1→L2 messages. If a bridge requiring ETH is added, unstaking from Treasury would fail. |
9999
| **Recommendation** | Add `{value: ...}` forwarding or document that future bridges must not require ETH for L1→L2 UNSTAKE messages |
100+
| **Resolution** | Fixed. `requestToWithdraw` now validates `msg.value`, tracks `remainingValue`, and forwards `{value: externalValue}` to `unstakeExternal` and `{value: remainingValue}` to `unstake`. Reverts if leftover ETH remains. |
100101

101-
#### INFO-2: Dangling OLAS Approval in Distributor
102+
#### INFO-2: Dangling OLAS Approval in Distributor — FIXED
102103
| | |
103104
|---|---|
104105
| **Location** | Distributor.sol:75 |
105106
| **Description** | If `Lock.increaseLock()` fails via low-level call, OLAS approval to Lock remains until next `distribute()` call |
106107
| **Impact** | None — Lock is immutable trusted address. Approval overwritten on next distribute(). |
107108
| **Recommendation** | Consider resetting approval to 0 after failed lock call |
109+
| **Resolution** | Fixed. Added `IToken(olas).approve(lock, 0)` in the failure branch of `_increaseLock()`. |
108110

109-
#### INFO-3: ERC6909 Withdrawal Tokens Are Transferable
111+
#### INFO-3: ERC6909 Withdrawal Tokens Are Transferable — by design
110112
| | |
111113
|---|---|
112114
| **Location** | Treasury.sol:190, solmate ERC6909 |
113115
| **Description** | Withdrawal claim tokens can be transferred. New holder can finalize withdrawal after delay. |
114116
| **Impact** | Feature, not bug. Enables secondary market for withdrawal claims. |
115117
| **Recommendation** | Document this behavior for users |
116118

117-
#### INFO-4: Multiple Permissionless Trigger Functions
119+
#### INFO-4: Multiple Permissionless Trigger Functions — by design
118120
| | |
119121
|---|---|
120122
| **Location** | Depository.sol:763,829; Distributor.sol:125; UnstakeRelayer.sol:60; Collector.sol:317; ActivityModule.sol:303 |

0 commit comments

Comments
 (0)