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 vaultEIP20compatible?SC2- Doesassetreturn the address of the token that is deposited in the vault?SC3- IsassetaERC20compatible?
SC4- Doesassetnever revert?SC5- DoestotalAssetsalways include the compounding that occurs from yield?SC6- DoestotalAssetsalways include any fees externally charged to the vault?SC7- DoestotalAssetsnever revert?
SC8- DoconvertToAssets&convertToSharesalways include any fees externally charged to the vault?SC9- DoconvertToAssets&convertToSharesnever vary depending on the caller?SC10- DoconvertToAssets&convertToSharesnever exclude slippage,vault fees and other chain conditions?SC11- DoconvertToAssets&convertToSharesnever revert, except for a overflow due to a big integer input or similar?SC12- DoconvertToAssets&convertToSharesalways round down?
SC13- DoesmaxDepositalways return maximum amount of assetsdepositwould allow to be deposited forreceiver?SC14- DoesmaxMintalways return maximum amount of sharesmintwould allow to be minted forreceiver?SC15- DomaxDeposit&maxMintalways return0if deposits are disabled?SC16- DomaxDeposit&maxMintalways return2 ** 256 - 1when there is no limit?SC17- DomaxDeposit&maxMintnever relay onbalanceOfofasset?SC18- DomaxDeposit&maxMintnever revert?
SC19- DoespreviewDepositalways ignore themaxDeposit?SC20- DoespreviewMintalways ignore themaxMint?SC21- DoespreviewDepositalways return same or fewer shares than those that would actually be minted ondeposit?SC22- DoespreviewMintalways return same or more assets than those that would actually be deposited onmint?SC23- DopreviewDeposit&previewMintalways include any vault deposit fees ?SC24- DopreviewDeposit&previewMintnever revert, except for a overflow due to a big integer input or similar?SC25- DopreviewDeposit&previewMintalways consider slippage and other chain conditions unlikeconvertToShares&convertToAssets?SC26- DoespreviewDepositalways round down ?SC27- DoespreviewMintalways round up ?
SC28- Doesdepositalways pull exactlyassetstokens?SC29- Doesmintalways mint exactlysharesshares?SC30- Doesdepositalways mint exactlysharestokens?SC31- Doesmintalways return the exact amount ofassetsdeposited?SC32- Doesdepositalways revert if themaxDepositis exceeded?SC33- Doesmintalways revert if themaxMintis exceeded?SC34- Dodeposit&mintalways support theapprove/transferFromonassetas a deposit flow?SC35- Dodeposit&mintalways emit theDepositevent?
SC36- DoesmaxWithdrawalways return the maximum amount of assets that a user canwithdraw?SC37- DoesmaxRedeemalways return the maximum amount of shares that a user canredeem?SC38- DomaxWithdraw&maxRedeemalways return0if withdrawals are disabled?SC39- DomaxWithdraw&maxRedeemnever revert?
SC40- DoespreviewWithdrawalways round up?SC41- DoespreviewRedeemalways round down?SC42- DoespreviewWithdrawalways return same or moresharesthan those that would actually be burnt when performing awithdraw?SC43- DoespreviewWithdrawalways return same or fewerassetsthan those that would actually be sent to the user when performing awithdraw?SC44- DoespreviewWithdrawalways ignore themaxWithdraw?SC45- DoespreviewRedeenalways ignore themaxRedeem?SC47- DopreviewWithdraw&previewRedeenalways include any vault withdrawal fee?SC48- DopreviewWithdraw&previewRedeenalways consider slippage and other chain conditions unlikeconvertToShares&convertToAssets?SC49- DopreviewWithdraw&previewRedeennever revert except for other conditions that would make thewithdraw&redeemrevert?
SC50- Doeswithdrawalways transfer exactlyassetsassets?SC51- Doesredeemalways burn exactlysharesshares?SC52- Doeswithdrawalways burn exactlysharesshares?SC53- Doesredeemalways transfer exactlyassetsassets?SC54- Doeswithdrawalways revert ifassetsexceedsmaxMint?SC55- Doesredeemalways revert ifsharesexceedsmaxRedeem?SC56- Dowithdrawandredeemalways allow to operate on behalf of anotheroperatoras long as there is an approval of his shares to themsg.senderthat is decreased after the call?SC57- Dowithdrawandredeemalways emit theWithdrawfunction?
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 thesuccessreturn 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- AretotalAssetspesimistically 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.soland 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::SafeERC20to 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?
