Skip to content

Commit b235da0

Browse files
kupermindkupermind
authored
Merge pull request #16 from LemonTreeTechnologies/post-audit-review
Post audit review
2 parents 46f5737 + ea32ecb commit b235da0

2 files changed

Lines changed: 129 additions & 0 deletions

File tree

‎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.**

0 commit comments

Comments
 (0)