Skip to content

doc: internal audit8 #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kupermind merged 3 commits into from
Mar 23, 2026
Merged

doc: internal audit8 #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a4f62d1
doc: internal audit8
77ph Mar 8, 2026
046d0f9
doc: internal audit8 - remove internal files
77ph Mar 8, 2026
7be90f3
doc: internal audit9 — full project re-audit + delta review
77ph Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion audits/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ An `audit3` with a focus `post-internal-audit` branch [audit3](https://github.co
An `audit4` with a focus `post-lock` branch [audit4](https://github.com/kupermind/olas-lst/blob/main/audits/audit4). <br>
An `audit5` with a focus `main` branch [audit5](https://github.com/kupermind/olas-lst/blob/main/audits/audit5). <br>
An `audit6` with a focus `main` branch [audit6](https://github.com/kupermind/olas-lst/blob/main/audits/audit6). <br>
An `audit7` with a focus `main` branch [audit6](https://github.com/LemonTreeTechnologies/olas-lst/blob/main/audits/audit7). <br>
An `audit7` with a focus `main` branch [audit7](https://github.com/LemonTreeTechnologies/olas-lst/blob/main/audits/audit7). <br>
An `audit8` with a focus `main` branch [audit8](https://github.com/LemonTreeTechnologies/olas-lst/blob/main/audits/audit8). <br>
An `audit9` — full project re-audit + delta review [audit9](https://github.com/LemonTreeTechnologies/olas-lst/blob/main/audits/audit9). <br>

### External audits
External audit reports are listed in their historical order:
Expand Down
174 changes: 174 additions & 0 deletions audits/audit8/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
# Security Audit Report — olas-lst (stOLAS)

**Date**: 2026-03-08
**Scope**: All contracts in `contracts/` (excluding `lib/`, `test/`, `contracts/l1/concept/`, `contracts/test/`)
**Lines of Code**: 8,086 LOC across 29 contracts

---

## Executive Summary

The stOLAS liquid staking protocol was audited following the 5-phase security audit methodology. The codebase implements a cross-chain ERC4626 vault system with L1 (Ethereum) deposits/withdrawals and L2 (Gnosis/Base) staking operations.

**Result: No bounty-qualifying vulnerabilities found.**

The protocol demonstrates mature defensive coding patterns:
- Internal accounting model (totalReserves) immune to donation-based price manipulation
- Time-delayed withdrawals preventing flash loan and sandwich attacks
- Per-contract reentrancy guards (transient storage on L1, persistent on L2)
- Strict access control with one-caller-per-function pattern on stOLAS
- Optimistic cross-chain accounting with recovery mechanisms for failed bridge messages

4 informational observations were documented. The protocol has undergone 7 internal and 1 external audit (CODESPECT), and the code quality reflects this maturity.

---

## Scope

### In-Scope Contracts

**L1 (Ethereum) — 2,633 LOC**
| Contract | LOC | Description |
|----------|-----|-------------|
| stOLAS.sol | 378 | ERC4626 vault, main entry point |
| Depository.sol | 1,139 | Staking model management, cross-chain bridge hub |
| Treasury.sol | 303 | Withdrawal queue, ERC6909 claim tokens |
| Distributor.sol | 148 | Reward split (veOLAS lock + vault) |
| Lock.sol | 287 | veOLAS lock management, governance proxy |
| UnstakeRelayer.sol | 80 | Retired model unstake relay |

**L1 Bridging — 738 LOC**
| Contract | LOC | Description |
|----------|-----|-------------|
| DefaultDepositProcessorL1.sol | 182 | Abstract bridge base |
| BaseDepositProcessorL1.sol | 116 | Optimism/Base bridge |
| GnosisDepositProcessorL1.sol | 62 | Gnosis AMB bridge |
| LzOracle.sol | 378 | LayerZero lzRead oracle (NOT currently used) |

**L2 (Gnosis/Base) — 3,851 LOC**
| Contract | LOC | Description |
|----------|-----|-------------|
| StakingManager.sol | 556 | Service orchestration |
| ExternalStakingDistributor.sol | 1,123 | External staking management |
| StakingTokenLocked.sol | 848 | Locked staking instance |
| ActivityModule.sol | 342 | Service activity + claim |
| Collector.sol | 406 | Reward collection + bridging |
| StakingHelper.sol | 77 | View helper |
| MultisigGuard.sol | 216 | Safe guard for externals |
| ModuleActivityChecker.sol | 33 | Activity nonce checker |

**L2 Bridging — 679 LOC**
| Contract | LOC | Description |
|----------|-----|-------------|
| DefaultStakingProcessorL2.sol | 473 | Abstract bridge base |
| BaseStakingProcessorL2.sol | 113 | Optimism/Base bridge |
| GnosisStakingProcessorL2.sol | 93 | Gnosis bridge |

**Infrastructure — 255 LOC**
| Contract | LOC | Description |
|----------|-----|-------------|
| Beacon.sol | 62 | Beacon pattern |
| BeaconProxy.sol | 60 | EIP-1967 beacon proxy |
| Proxy.sol | 73 | UUPS-style proxy |
| Implementation.sol | 60 | Base implementation with owner |

### Out of Scope
- `lib/` dependencies (autonolas-registries, LayerZero, OpenZeppelin, solmate)
- `contracts/test/` mock contracts
- `contracts/l1/concept/` experimental contracts
- Owner/multisig actions (trusted role per CLAUDE.md)
- LzOracle.sol (confirmed not currently used)

---

## Findings

### Critical: 0
### High: 0
### Medium: 0
### Low: 0

### Informational: 4

#### INFO-1: Missing ETH Forwarding in Treasury.requestToWithdraw
| | |
|---|---|
| **Location** | Treasury.sol:220-221, 227-228 |
| **Description** | `requestToWithdraw` is `payable` but doesn't forward `msg.value` to `Depository.unstake`/`unstakeExternal` calls |
| **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. |
| **Recommendation** | Add `{value: ...}` forwarding or document that future bridges must not require ETH for L1→L2 UNSTAKE messages |

#### INFO-2: Dangling OLAS Approval in Distributor
| | |
|---|---|
| **Location** | Distributor.sol:75 |
| **Description** | If `Lock.increaseLock()` fails via low-level call, OLAS approval to Lock remains until next `distribute()` call |
| **Impact** | None — Lock is immutable trusted address. Approval overwritten on next distribute(). |
| **Recommendation** | Consider resetting approval to 0 after failed lock call |

#### INFO-3: ERC6909 Withdrawal Tokens Are Transferable
| | |
|---|---|
| **Location** | Treasury.sol:190, solmate ERC6909 |
| **Description** | Withdrawal claim tokens can be transferred. New holder can finalize withdrawal after delay. |
| **Impact** | Feature, not bug. Enables secondary market for withdrawal claims. |
| **Recommendation** | Document this behavior for users |

#### INFO-4: Multiple Permissionless Trigger Functions
| | |
|---|---|
| **Location** | Depository.sol:763,829; Distributor.sol:125; UnstakeRelayer.sol:60; Collector.sol:317; ActivityModule.sol:303 |
| **Description** | These functions (unstakeRetired, closeRetiredStakingModels, distribute, relay, relayTokens, claim) can be called by anyone. |
| **Impact** | None — all are designed as permissionless triggers. Cannot cause fund loss when called by untrusted parties. |
| **Recommendation** | N/A — by design |

---

## Methodology

### Phase 1: Architecture Map
- Mapped 29 in-scope contracts across L1, L2, bridging, and infrastructure layers
- Documented inheritance hierarchy, inter-contract call graph, access control matrix
- Identified 10 priority attack surface areas

### Phase 2: Invariant Identification
- Verified 10 core invariants (all holding)
- Disproved 8 initial candidate findings through code verification
- Identified 8 candidates for Phase 3 deep investigation

### Phase 3: Attack Discovery
- Investigated all 8 Phase 2 candidates — all disproved
- Ran 16-item EVM checklist — all SAFE
- Ran 8-item DeFi attack pattern checklist — all SAFE
- Applied disprove-first methodology to every finding

### Coverage

| Category | Items Checked |
|----------|--------------|
| Phase 2 candidates | 8/8 investigated |
| EVM patterns | 16/16 checked |
| DeFi patterns | 8/8 checked |
| Invariants verified | 10/10 |
| False positives disproved | 17 |
| Informational findings | 4 |

---

## Key Architectural Observations

### Strengths
1. **Internal accounting model**: `totalAssets()` returns `totalReserves` (not `balanceOf`), making the vault immune to donation-based attacks (ERC4626 inflation, share price manipulation)
2. **Withdrawal delay**: Time-delayed withdrawals via Treasury prevent flash loans, sandwich attacks, and cross-chain arbitrage
3. **Access control isolation**: Each stOLAS function has exactly one authorized caller — no cross-role attack vectors
4. **Graceful degradation**: Distributor handles Lock failure gracefully (all rewards go to vault); L2 bridge queues failed operations for retry
5. **Rounding direction**: Both deposit and redeem use `mulDivDown` (favor vault), preventing rounding extraction

### Design Trade-offs (Not Vulnerabilities)
1. **Optimistic cross-chain accounting**: L1 updates balances before L2 confirms. Relies on operational liveness for recovery.
2. **No virtual shares**: stOLAS doesn't implement OpenZeppelin's virtual shares offset. Mitigated by depository-only deposit access control.
3. **ERC4626 non-standard functions**: `mint()` and `withdraw()` revert. May confuse integrators.

---

*Audit conducted using the Security Audit Playbook v2.8. All findings were verified using disprove-first methodology with exact file:line references.*
142 changes: 142 additions & 0 deletions audits/audit8/external-staking-distributor-analysis.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
# Detailed Analysis: ExternalStakingDistributor.sol (1,123 LOC)

**Date**: 2026-03-08
**Contract**: `contracts/l2/ExternalStakingDistributor.sol`

---

## Architecture Overview

The contract manages external staking services on L2 — creates Safe multisig wallets, stakes services in external staking proxies, handles reward distribution, and processes cross-chain unstake requests.

**Key roles:**
- `owner` — configures proxies, agents
- `l2StakingProcessor` — bridge endpoint (deposit/withdraw)
- `managingAgents` — can trigger unstake
- curating agents — can stake services (per-proxy access via `stakingGuard`)
- anyone — can call `claim`, `unstakeAndWithdraw` if evicted/no rewards

---

## Detailed Findings

### 1. `reStake` uses dead `mapCuratingAgents` mapping (lines 804, 244-245)

**Line 244:** `// UNUSED for now: Mapping whitelisted curating agent addresses`
**Line 245:** `mapping(address => bool) public mapCuratingAgents;`

**Line 804 (reStake):**
```solidity
if (!(mapCuratingAgents[msg.sender] || mapManagingAgents[msg.sender] || msg.sender == owner))
```

**But `stake` (line 610) uses:**
```solidity
curatingAgentAccess = mapStakingGuardHashCuratingAgents[stakingHash][msg.sender];
```

No function ever sets `mapCuratingAgents` to `true` — `setCuratingAgents` (line 912) writes to `mapStakingGuardHashCuratingAgents` instead. So the curating agent access check in `reStake` is dead code.

**Impact:** A curating agent who staked a service cannot restake it after eviction — only managing agents or owner can. No fund loss — managing agents/owner can still restake. Functionality limitation only.

**Severity:** Informational. The "UNUSED for now" comment suggests intentional deferral.

---

### 2. Reward distribution math — verified correct (lines 494-500)

```solidity
collectorAmount = (reward * collectorAmount) / MAX_REWARD_FACTOR; // round DOWN
protocolAmount = (reward * protocolAmount) / MAX_REWARD_FACTOR; // round DOWN
uint256 fullCollectorAmount = collectorAmount + protocolAmount;
uint256 curatingAgentAmount = reward - fullCollectorAmount; // remainder → curating agent
```

Max dust loss: 2 wei per distribution (two round-downs). Curating agent absorbs remainder. Total always equals `reward`. **No issue.**

---

### 3. V1 DelegateCall to multiSend — verified safe (line 543)

```solidity
ISafe(multisig).execTransactionFromModule(multiSend, 0, msPayload, ISafe.Operation.DelegateCall);
```

Inner operations are all `ISafe.Operation.Call` (0x00). `multiSend` is immutable (line 220). MultiSendCallOnly rejects nested DelegateCall. **No issue.**

---

### 4. `unstakeAndWithdraw` permissionless path — verified safe (lines 697-702)

```solidity
if (stakingState == IStaking.StakingState.Staked && availableRewards > 0
&& !(mapManagingAgents[msg.sender] || msg.sender == owner)) {
revert UnauthorizedAccount(msg.sender);
}
```

Anyone can unstake if evicted OR `availableRewards == 0`. Rewards go to collector/protocol/curatingAgent — caller gets nothing. **By design.**

---

### 5. `withdrawAndRequestUnstake` deficit accumulation — verified correct (lines 1005-1009)

```solidity
if (amount > olasBalance) {
unstakeRequestedAmount = amount - olasBalance;
amount = olasBalance;
mapUnstakeOperationRequestedAmounts[operation] += unstakeRequestedAmount;
}
```

Deficit accumulates per operation. Fulfilled in `unstakeAndWithdraw` (lines 741-770) with partial fulfillment support. **Correct accounting.**

---

### 6. OLAS approve patterns — verified

- V2 line 553: `approve(collector, fullCollectorAmount)` → consumed by `topUpBalance(collectorAmount)` + `topUpProtocol(protocolAmount)`. Exact match.
- Line 764: `approve(collector, amount)` → consumed by `topUpBalance(amount)`. Exact match.
- Line 659: `approve(serviceRegistryTokenUtility, fullStakingDeposit)`. Consumed during deploy+stake. Correct.

No dangling approvals in normal flow. **No issue.**

---

### 7. `stakedBalance` accounting — verified

| Function | Operation | Line |
|----------|-----------|------|
| `stake` | `+= fullStakingDeposit` | 656 |
| `unstakeAndWithdraw` | `-= fullStakingDeposit` | 719 |
| `reStake` | no change (same deposit) | — |

Symmetric. `reStake` correctly leaves `stakedBalance` unchanged since it unstakes and restakes the same deposit. **No issue.**

---

### 8. `_createMultisigWithSelfAsModule` — verified safe (lines 338-401)

- Creates Safe with `address(this)` as initial owner
- Enables `address(this)` as module + guard as module
- Sets guard
- Swaps owner to `agentInstance`
- Uses contract signature (`r = bytes32(uint256(uint160(address(this))))`, `v = 1`)

After execution: agentInstance is owner, ExternalStakingDistributor and guard are modules, guard is set. **Correct setup.**

---

## Summary

| Area | Status |
|------|--------|
| Reward math | Safe — remainder absorbs rounding |
| DelegateCall | Safe — immutable target, Call-only inner ops |
| Balance tracking | Safe — symmetric stake/unstake |
| Access control | Functional — `reStake` curating agent check is dead code (informational) |
| Cross-chain deficit | Safe — accumulates and partially fulfills |
| Approve patterns | Safe — no dangling approvals |
| Reentrancy | Protected — `_locked` guard on all state-changing functions |

**No new bounty-qualifying vulnerabilities found.** The `mapCuratingAgents` dead code in `reStake` is consistent with what we already noted as a design observation — it's marked "UNUSED for now" by the developers.
Loading
Loading