-
Notifications
You must be signed in to change notification settings - Fork 11
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.
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
VIP-14 Morpho PCV Deposit #128
Conversation
…sconnect compound deposits from ERC20 allocator, connect new morpho deposits to ERC20 allocator
contracts/test/integration/IntegrationTestMaplePCVDeposit.t.sol
Outdated
Show resolved
Hide resolved
contracts/test/integration/IntegrationTestMaplePCVDeposit.t.sol
Outdated
Show resolved
Hide resolved
contracts/test/integration/IntegrationTestMaplePCVDeposit.t.sol
Outdated
Show resolved
Hide resolved
contracts/test/integration/IntegrationTestMorphoCompoundPCVDeposit.t.sol
Outdated
Show resolved
Hide resolved
contracts/test/integration/IntegrationTestMorphoCompoundPCVDeposit.t.sol
Show resolved
Hide resolved
contracts/test/integration/IntegrationTestMorphoCompoundPCVDeposit.t.sol
Outdated
Show resolved
Hide resolved
contracts/test/integration/IntegrationTestMorphoCompoundPCVDeposit.t.sol
Outdated
Show resolved
Hide resolved
contracts/test/integration/IntegrationTestMorphoCompoundPCVDeposit.t.sol
Outdated
Show resolved
Hide resolved
…o accomodate this, and update comments
…ents for organization
…pdates the liquid balance on the oracle, move balance cache to after supply
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work!
/// Updates the lastRecordedBalance to include all realized profits or losses. | ||
function _recordPNL() private { | ||
/// first accrue interest in Compound and Morpho | ||
IMorpho(morpho).updateP2PIndexes(cToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When calling withdraw or supply on Morpho the indexes are updated and this call might not be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is, we want to be able to know what the pcv deposit balance is including all unrealized gains or losses before depositing or withdrawing. To handle this, we must update the indexes first, which then means that when supply or withdraw is called on morpho, the interest indexes in morpho and compound don't get updated again.
if (pcvOracle != address(0)) { | ||
IPCVOracle(pcvOracle).updateLiquidBalance( | ||
endingRecordedBalance - startingRecordedBalance | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This piece of code is written 4 times and could have been in an internal function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could refactor this into a helper function, but it's too late in the game to change code before a deployment.
_recordPNL(); | ||
|
||
/// withdraw last recorded amount as this was updated in record pnl | ||
_withdraw(to, lastRecordedBalance, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a withdraw all, you also can pass type(uint256).max
as argument to avoid leaving dust on Morpho but as you're calling _recordPNL
just before it must be exactly the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing uint256 max to the withdraw function would revert as lastRecordedBalance has amount subtracted from it on line 309. This would cause an underflow and revert.
@@ -151,6 +167,14 @@ contract MorphoCompoundPCVDeposit is PCVDeposit, ReentrancyGuard { | |||
amount | |||
); | |||
|
|||
int256 endingRecordedBalance = balance().toInt256(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not read the state in lastRecordedBalance
? it's freshly updated with _recordPNL() etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lastRecordedBalance is off by a hair because on deposit, you take a loss when compound rounds down in favor of their protocol, so you have to read balance instead to get the correct amount of balance in the system.
|
||
_recordPNL(); | ||
|
||
IPCVOracle(pcvOracle).updateLiquidBalance( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can save an sload here by using _pcvOracle
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, looking good
@@ -0,0 +1,398 @@ | |||
// SPDX-License-Identifier: GNU AGPLv3 | |||
pragma solidity ^0.8.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for now, but in the vein of talking about how Morpho were impressive for locking down their npm package dependencies, might be worthwhile locking the Solidity version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, solidity version in Morpho is locked to 0.8.13, this file should have been locked as well.
address _morpho, | ||
address _lens | ||
) CoreRef(_core) ReentrancyGuard() { | ||
if (_underlying != address(Constants.WETH)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why's (_underlying != address(Constants.WETH))
there? I thought the underlying was USDC always?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we ever use this deposit for eth, the ceth contract has no underlying function and will revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
USDC and DAI, and in the future, could be other tokens.
@@ -19,7 +19,7 @@ contract PCVGuardVerification is DSTest { | |||
using Deviation for *; | |||
using SafeCast for *; | |||
|
|||
/// TODO add arbitrum support | |||
/// TODO add Morpho Deposits once deployed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flagging this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
… file, point vip14 solidity simulation to newly deployed smart contracts
This VIP will deposit all Compound deposits into Morpho, and add an allocation into a Maple venue. A Compound PCV Router will be deployed and hooked into the Morpho deposits so that switching between DAI and USDC can still happen while the protocol is in Morpho.
Audit Log
cToken.underlying()
Pre-Proposal Execution Steps
withdrawAllToSafeAddress
on the pcv guardian with the dai compound pcv depositPost-Proposal Execution Steps
Todo
Variables
V1
- Can it beinternal
?V2
- Can it beconstant
?V3
- Can it beimmutable
?V4
- Is its visibility set? (SWC-108)V5
- Is the purpose of the variable and other important information documented using natspec?V6
- Can it be packed with an adjacent storage variable?V7
- Can it be packed in a struct with more than 1 other variable?V8
- Use full 256 bit types unless packing with other variables.V9
- If it's a public array, is a separate function provided to return the full array?V10
- Only useprivate
to intentionally prevent child contracts from accessing the variable, preferinternal
for flexibility.Structs
S1
- Is a struct necessary? Can the variable be packed raw in storage?S2
- Are its fields packed together (if possible)?S3
- Is the purpose of the struct and all fields documented using natspec?Functions
F1
- Can it beexternal
?F2
- Should it beinternal
?F3
- Should it bepayable
?F4
- Can it be combined with another similar function?F5
- Validate all parameters are within safe bounds, even if the function can only be called by a trusted users.F6
- Is the checks before effects pattern followed? (SWC-107)F7
- Check for front-running possibilities, such as the approve function. (SWC-114)F8
- Is insufficient gas griefing possible? (SWC-126)F9
- Are the correct modifiers applied, such asonlyOwner
/requiresAuth
?F10
- Are return values always assigned?F11
- Write down and test invariants about state before a function can run correctly.F12
- Write down and test invariants about the return or any changes to state after a function has run.F13
- Take care when naming functions, because people will assume behavior based on the name.F14
- If a function is intentionally unsafe (to save gas, etc), use an unwieldy name to draw attention to its risk.F15
- Are all arguments, return values, side effects and other information documented using natspec?F16
- If the function allows operating on another user in the system, do not assumemsg.sender
is the user being operated on.F17
- If the function requires the contract be in an uninitialized state, check an explicitinitialized
variable. Do not useowner == address(0)
or other similar checks as substitutes.F18
- Only useprivate
to intentionally prevent child contracts from calling the function, preferinternal
for flexibility.F19
- Usevirtual
if there are legitimate (and safe) instances where a child contract may wish to override the function's behavior.Modifiers
M1
- Are no storage updates made (except in a reentrancy lock)?M2
- Are external calls avoided?M3
- Is the purpose of the modifier and other important information documented using natspec?Code
C1
- Using SafeMath or 0.8 checked math? (SWC-101)C2
- Are any storage slots read multiple times?C3
- Are any unbounded loops/arrays used that can cause DoS? (SWC-128)C4
- Useblock.timestamp
only for long intervals. (SWC-116)C5
- Don't use block.number for elapsed time. (SWC-116)C7
- Avoid delegatecall wherever possible, especially to external (even if trusted) contracts. (SWC-112)C8
- Do not update the length of an array while iterating over it.C9
- Don't useblockhash()
, etc for randomness. (SWC-120)C10
- Are signatures protected against replay with a nonce andblock.chainid
(SWC-121)C11
- Ensure all signatures use EIP-712. (SWC-117 SWC-122)C12
- Output ofabi.encodePacked()
shouldn't be hashed if using >2 dynamic types. Prefer usingabi.encode()
in general. (SWC-133)C13
- Careful with assembly, don't use any arbitrary data. (SWC-127)C14
- Don't assume a specific ETH balance. (SWC-132)C15
- Avoid insufficient gas griefing. (SWC-126)C16
- Private data isn't private. (SWC-136)C17
- Updating a struct/array in memory won't modify it in storage.C18
- Never shadow state variables. (SWC-119)C19
- Do not mutate function parameters.C20
- Is calculating a value on the fly cheaper than storing it?C21
- Are all state variables read from the correct contract (master vs. clone)?C22
- Are comparison operators used correctly (>
,<
,>=
,<=
), especially to prevent off-by-one errors?C23
- Are logical operators used correctly (==
,!=
,&&
,||
,!
), especially to prevent off-by-one errors?C24
- Always multiply before dividing, unless the multiplication could overflow.C25
- Are magic numbers replaced by a constant with an intuitive name?C26
- If the recipient of ETH had a fallback function that reverted, could it cause DoS? (SWC-113)C27
- Use SafeERC20 or check return values safely.C28
- Don't usemsg.value
in a loop.C29
- Don't usemsg.value
if recursive delegatecalls are possible (like if the contract inheritsMulticall
/Batchable
).C30
- Don't assumemsg.sender
is always a relevant user.C31
- Don't useassert()
unless for fuzzing or formal verification. (SWC-110)C32
- Don't usetx.origin
for authorization. (SWC-115)C33
- Don't useaddress.transfer()
oraddress.send()
. Use.call.value(...)("")
instead. (SWC-134)C34
- When using low-level calls, ensure the contract exists before calling.C35
- When calling a function with many parameters, use the named argument syntax.C36
- Do not use assembly for create2. Prefer the modern salted contract creation syntax.C37
- Do not use assembly to access chainid or contract code/size/hash. Prefer the modern Solidity syntax.C38
- Use thedelete
keyword when setting a variable to a zero value (0
,false
,""
, etc).C39
- Comment the "why" as much as possible.C40
- Comment the "what" if using obscure syntax or writing unconventional code.C41
- Comment explanations + example inputs/outputs next to complex and fixed point math.C42
- Comment explanations wherever optimizations are done, along with an estimate of much gas they save.C43
- Comment explanations wherever certain optimizations are purposely avoided, along with an estimate of much gas they would/wouldn't save if implemented.C44
- Useunchecked
blocks where overflow/underflow is impossible, or where an overflow/underflow is unrealistic on human timescales (counters, etc). Comment explanations whereverunchecked
is used, along with an estimate of how much gas it saves (if relevant).C45
- Do not depend on Solidity's arithmetic operator precedence rules. In addition to the use of parentheses to override default operator precedence, parentheses should also be used to emphasise it.C46
- Expressions passed to logical/comparison operators (&&
/||
/>=
/==
/etc) should not have side-effects.C47
- Wherever arithmetic operations are performed that could result in precision loss, ensure it benefits the right actors in the system, and document it with comments.C48
- Document the reason why a reentrancy lock is necessary whenever it's used with an inline or@dev
natspec comment.C49
- When fuzzing functions that only operate on specific numerical ranges use modulo to tighten the fuzzer's inputs (such asx = x % 10000 + 1
to restrict from 1 to 10,000).C50
- Use ternary expressions to simplify branching logic wherever possible.C51
- When operating on more than one address, ask yourself what happens if they're the same.External Calls
X1
- Is an external contract call actually needed?X2
- If there is an error, could it cause DoS? LikebalanceOf()
reverting. (SWC-113)X3
- Would it be harmful if the call reentered into the current function?X4
- Would it be harmful if the call reentered into another function?X5
- Is the result checked and errors dealt with? (SWC-104)X6
- What if it uses all the gas provided?X7
- Could it cause an out-of-gas in the calling contract if it returns a massive amount of data?X8
- If you are calling a particular function, do not assume thatsuccess
implies that the function exists (phantom functions).Static Calls
S1
- Is an external contract call actually needed?S2
- Is it actually marked as view in the interface?S3
- If there is an error, could it cause DoS? LikebalanceOf()
reverting. (SWC-113)S4
- If the call entered an infinite loop, could it cause DoS?Events
E1
- Should any fields be indexed?E2
- Is the creator of the relevant action included as an indexed field?E3
- Do not index dynamic types like strings or bytes.E4
- Is when the event emitted and all fields documented using natspec?E5
- Are all users/ids that are operated on in functions that emit the event stored as indexed fields?E6
- Avoid function calls and evaluation of expressions within event arguments. Their order of evaluation is unpredictable.Contract
T1
- Use an SPDX license identifier.T2
- Are events emitted for every storage mutating function?T3
- Check for correct inheritance, keep it simple and linear. (SWC-125)T4
- Use areceive() external payable
function if the contract should accept transferred ETH.T5
- Write down and test invariants about relationships between stored state.T6
- Is the purpose of the contract and how it interacts with others documented using natspec?T7
- The contract should be markedabstract
if another contract must inherit it to unlock its full functionality.T8
- Emit an appropriate event for any non-immutable variable set in the constructor that emits an event when mutated elsewhere.T9
- Avoid over-inheritance as it masks complexity and encourages over-abstraction.T10
- Always use the named import syntax to explicitly declare which contracts are being imported from another file.T11
- Group imports by their folder/package. Separate groups with an empty line. Groups of external dependencies should come first, then mock/testing contracts (if relevant), and finally local imports.T12
- Summarize the purpose and functionality of the contract with a@notice
natspec comment. Document how the contract interacts with other contracts inside/outside the project in a@dev
natspec comment.Project
P1
- Unit test everything.P2
- Fuzz test as much as possible.P3
- Use symbolic execution where possible.P4
- Run Slither/Solhint and review all findings.DeFi
D1
- Check your assumptions about what other contracts do and return.D2
- Don't mix internal accounting with actual balances.D3
- Don't use spot price from an AMM as an oracle.D4
- Do not trade on AMMs without receiving a price target off-chain or via an oracle.D5
- Use sanity checks to prevent oracle/price manipulation.D6
- Watch out for rebasing tokens. If they are unsupported, ensure that property is documented.D7
- Watch out for ERC-777 tokens. Even a token you trust could preform reentrancy if it's an ERC-777.D8
- Watch out for fee-on-transfer tokens. If they are unsupported, ensure that property is documented.D9
- Watch out for tokens that use too many or too few decimals. Ensure the max and min supported values are documented.D10
- Be careful of relying on the raw token balance of a contract to determine earnings. Contracts which provide a way to recover assets sent directly to them can mess up share price functions that rely on the raw Ether or token balances of an address.D11
- If your contract is a target for token approvals, do not make arbitrary calls from user input.PR Specific Security Areas
withdrawERC20ToSafeAddress
andwithdrawAllERC20ToSafeAddress
will not work with removing Morpho Tokens on the Morpho PCV Deposit because Morpho has no concept of mTokens. This means if the contract is paused, or an issue is surfaced in Morpho and liquidity is locked, Volt will need to rely on social coordination with the Morpho team to recover funds.