Glacier audit
A private security review of Glacier (liquid staking), conducted with Three Sigma and completed on 12th July 2023 over 8 days. 0xSimao disclosed 32 findings: 6 high severity, 3 medium, 4 low and 19 informational.
What Glacier is
Glacier is a liquid staking protocol on Avalanche: users deposit any amount of AVAX, the protocol stakes it with network validators, and the position stays liquid, rewards accruing hourly, with reserves split across venues including Aave to keep exits serviceable.
Scope
The review covered the reserve and rebalance machinery: how per-venue reserves are fetched and summed, share pricing on withdrawal, wAVAX unwrapping in rebalances, and behaviour when a lending reserve freezes.
High severity findings
- AaveV3 RewardsController provides rewards in any token, should be handled separately
- totalReserves should be fetched from the strategies individually and summed up in the ReservePool
- increaseNetworkTotal() allows big arbitrage opportunities by depositing before an increase transaction
- _rebalanceWithdraw() mechanism in glAVAX allows arbitrage opportunities by changing the shares/AVAX ratio
- In glAVAX, function _rebalanceWithdraw() withdraws incorrect amount from WAVAX address
- In GReservePool, if a strategy is frozen reserve pool stops working
Medium severity findings
- When changing addresses, use 2 step transfer and/or address 0x0 checks
- Strategy withdraw may fail if weights of strategies differ from the real values and might lead to frozen ReservePool
- Withdraw snapshot logic can be tricked allowing users to withdraw right away
Low severity findings
- In glAVAX, should use .call instead of .transfer
- Strategy percentages will differ over time as yield accrued differs in ReservePool
- rebalance() in glAVAX reverts if currentReserves == reserveTarget
- receive() in glAVAX should only allow wAVAX
Informational findings
- Usage of transferFrom() could revert if used from itself
- Strategies in the ReservePool could be implemented as an array and Strategy packed
- Implement _transferShares() to prevent having to convert shares/wAVAX twice in glAVAX
- rebalance(), withdrawAmount needed from the ReservePool can be simplified
- Checks effects interactions pattern should always be used
- If statements can be inverted to increase readability
- avaxAmount is never 0 in withdraw() in the first if (avaxAmount > 0 && ...)
- Withdraw requests could be stored in a simpler way in glAVAX
- In glAVAX, checking that amount > 0 earlier can save some gas
- In glAVAX, naming convention for storage variable should be consistent
- Storage variables should be cached whenever possible to save gas
- Unnecessary user balance check in _withdrawRequest()
- Typo in fufillWithdrawal() in glAVAX, should be fulfillWithdrawal
- In GReservePool, remove unnecessary logic
- In GReservePool, use constants naming conventions
- In GLendingPool, remove incorrect comment
- In GLendingPool, event parameters can be indexed
- Remove unused imports
- NatSpect comments should include explanations of parameters and return variables
The report