Comprehensive Security Checklist for ERC4626 Standardized Yield Farming Vaults: A meticulously curated guide outlining essential security measures for developers engaging with the ERC4626 standard. This checklist offers a thorough examination of best practices and precautions to fortify the security posture of yield farming vaults, ensuring robust protection against potential threats and vulnerabilities
Created by @solthodox and @MaslarovK
- Read the project's docs, specs, and whitepaper to understand what the smart contracts are meant to do.
- Construct a mental model of what you expect the contracts to look like before checking out the code.
- Do a generic line-by-line review of the contracts.
- Do another review from the perspective of every actor in the threat model.
- Glance over the project's tests + code coverage and look deeper at areas lacking coverage.
- Look at related projects and their audits to check for any similar issues or oversights.
SC1
- Is the vaultEIP20
compatible?SC2
- Doesasset
return the address of the token that is deposited in the vault?SC3
- Isasset
aERC20
compatible?
SC4
- Doesasset
never revert?SC5
- DoestotalAssets
always include the compounding that occurs from yield?SC6
- DoestotalAssets
always include any fees externally charged to the vault?SC7
- DoestotalAssets
never revert?
SC8
- DoconvertToAssets
&convertToShares
always include any fees externally charged to the vault?SC9
- DoconvertToAssets
&convertToShares
never vary depending on the caller?SC10
- DoconvertToAssets
&convertToShares
never exclude slippage,vault fees and other chain conditions?SC11
- DoconvertToAssets
&convertToShares
never revert, except for a overflow due to a big integer input or similar?SC12
- DoconvertToAssets
&convertToShares
always round down?
SC13
- DoesmaxDeposit
always return maximum amount of assetsdeposit
would allow to be deposited forreceiver
?SC14
- DoesmaxMint
always return maximum amount of sharesmint
would allow to be minted forreceiver
?SC15
- DomaxDeposit
&maxMint
always return0
if deposits are disabled?SC16
- DomaxDeposit
&maxMint
always return2 ** 256 - 1
when there is no limit?SC17
- DomaxDeposit
&maxMint
never relay onbalanceOf
ofasset
?SC18
- DomaxDeposit
&maxMint
never revert?
SC19
- DoespreviewDeposit
always ignore themaxDeposit
?SC20
- DoespreviewMint
always ignore themaxMint
?SC21
- DoespreviewDeposit
always return same or fewer shares than those that would actually be minted ondeposit
?SC22
- DoespreviewMint
always return same or more assets than those that would actually be deposited onmint
?SC23
- DopreviewDeposit
&previewMint
always include any vault deposit fees ?SC24
- DopreviewDeposit
&previewMint
never revert, except for a overflow due to a big integer input or similar?SC25
- DopreviewDeposit
&previewMint
always consider slippage and other chain conditions unlikeconvertToShares
&convertToAssets
?SC26
- DoespreviewDeposit
always round down ?SC27
- DoespreviewMint
always round up ?
SC28
- Doesdeposit
always pull exactlyassets
tokens?SC29
- Doesmint
always mint exactlyshares
shares?SC30
- Doesdeposit
always mint exactlyshares
tokens?SC31
- Doesmint
always return the exact amount ofassets
deposited?SC32
- Doesdeposit
always revert if themaxDeposit
is exceeded?SC33
- Doesmint
always revert if themaxMint
is exceeded?SC34
- Dodeposit
&mint
always support theapprove
/transferFrom
onasset
as a deposit flow?SC35
- Dodeposit
&mint
always emit theDeposit
event?
SC36
- DoesmaxWithdraw
always return the maximum amount of assets that a user canwithdraw
?SC37
- DoesmaxRedeem
always return the maximum amount of shares that a user canredeem
?SC38
- DomaxWithdraw
&maxRedeem
always return0
if withdrawals are disabled?SC39
- DomaxWithdraw
&maxRedeem
never revert?
SC40
- DoespreviewWithdraw
always round up?SC41
- DoespreviewRedeem
always round down?SC42
- DoespreviewWithdraw
always return same or moreshares
than those that would actually be burnt when performing awithdraw
?SC43
- DoespreviewWithdraw
always return same or fewerassets
than those that would actually be sent to the user when performing awithdraw
?SC44
- DoespreviewWithdraw
always ignore themaxWithdraw
?SC45
- DoespreviewRedeen
always ignore themaxRedeem
?SC47
- DopreviewWithdraw
&previewRedeen
always include any vault withdrawal fee?SC48
- DopreviewWithdraw
&previewRedeen
always consider slippage and other chain conditions unlikeconvertToShares
&convertToAssets
?SC49
- DopreviewWithdraw
&previewRedeen
never revert except for other conditions that would make thewithdraw
&redeem
revert?
SC50
- Doeswithdraw
always transfer exactlyassets
assets?SC51
- Doesredeem
always burn exactlyshares
shares?SC52
- Doeswithdraw
always burn exactlyshares
shares?SC53
- Doesredeem
always transfer exactlyassets
assets?SC54
- Doeswithdraw
always revert ifassets
exceedsmaxMint
?SC55
- Doesredeem
always revert ifshares
exceedsmaxRedeem
?SC56
- Dowithdraw
andredeem
always allow to operate on behalf of anotheroperator
as long as there is an approval of his shares to themsg.sender
that is decreased after the call?SC57
- Dowithdraw
andredeem
always emit theWithdraw
function?
M1
- Are all the roundings done in favor of the protocol to prevent any rounding vulnerability?M2
- Are the multiplications done before the divisions?M3
- Is the contract using a math library(e.g.OZ::Math
) to make sure big operations don't overflow?M4
- Can any value be maliciously inflated by sending assets to the vault?M5
- If there is any entry or exit fee, is the inverse of the fee properly calculated when converting from assets to shares or vice versa?M6
- If there is any decimal scalation, is it correctly done?M7
- Can the vault properly handle the cases where there are zero shares, assets or even both?
E1
- Does the vault properly handle reverting calls to external dependencies?E2
- Is the vault susceptible to suffering a gas grieffing due to this external calls?E3
- Does the vault dangerously set a fixed gas limit when doing external calls?E4
- If performing raw calls, does the contract check for thesuccess
return value?E5
- If performing swaps, is the minimum tokens out parameter set to 0 to make sure the swaps always suceeds?E6
- If the vault incurs in losses in withdrawals because of slippage in swaps, does it have some kind of TVL limit so in case all the assets need to be liquidated the slippage is not crazy?
SP1
- AretotalAssets
pesimistically accounted?SP2
- Does the share price relay too much on external dependencies?SP3
- Can the share price be manipulated by either vault users or vault's external dependencies(e.g. oracle manipulation)?SP4
- Is the vault share price inflation attack safe?SP5
- If there is any profit lock mechanism, is it correctly reflected in the share price?
IC1
- If the contracts inherits fromERC4626.sol
and modifies some logic from the original implementation, does it override all of the needed functions from the inheriting for the vault to work properly?IC2
- Is there any storage collision due to the inheritance?
T1
- If the vault is intended to use any token, does it properly work with fee-on-transfer or rebase tokens?T2
- Can some malicious token reenter the vault?T2
- Can the contract be DoSed because an approval race conditon in the underlyig token?T2
- If the vault is intended to use any token, does it use a safe transfers libary such asOZ::SafeERC20
to safely interact with those that dont have a return value or other cases?
P1
- Are there any constraints when it comes to fees or other values that can be set by a trusted role?P2
- Can a trusted role steal funds from the vault?P3
- Will users' funds be locked if the vault is paused/shutdown?P4
- Do functions have a solid input validation?P6
- If the funds are invested in external protocols, does the vault have some kind of emergency mode where the needed positions can be fully liquidated if neeed?
G1
- Does the contract try to keep the logic simple?G2
- Does the contract implement battle-tested code?G3
- Does the contract use a safe soliditypragma
?G4
- Do tests have a high(>=90%) code coverage?G5
- If implementing external protocols, did the doing following the recommendations in the documentation?G6
- Does the test suite have fuzz-tests as well?