-
Notifications
You must be signed in to change notification settings - Fork 30
[Certora] Prove revert conditions for allocate, deallocate, setName, setSymbol #877
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
base: main
Are you sure you want to change the base?
Conversation
…e have an added assumption that safeTransfer/From is not reverting
|
|
||
| require forall uint256 i. i < ids.length => currentContract.caps[ids[i]].allocation <= 2 ^ 255 - 1, "assume allocation is small enough to cast to int256"; | ||
| require forall uint256 i. i < ids.length => currentContract.caps[ids[i]].allocation + change >= 0, "see changeForAllocateOrDeallocateIsBoundedByAllocation"; | ||
| require forall uint256 i. i < ids.length => currentContract.caps[ids[i]].allocation + change <= currentContract.caps[ids[i]].absoluteCap, "assume updated allocation respects absolute cap"; |
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.
IIUC this can not be proved using the assumption that allocation <= absoluteCap. TBC
| require forall uint256 i. i < ids.length => currentContract.caps[ids[i]].allocation > 0, "assume allocation is positive"; | ||
| require forall uint256 i. i < ids.length => currentContract.caps[ids[i]].allocation <= 2 ^ 255 - 1, "assume allocation is small enough to cast to int256"; | ||
| require forall uint256 i. i < ids.length => currentContract.caps[ids[i]].allocation + change >= 0, "see changeForAllocateOrDeallocateIsBoundedByAllocation"; | ||
| require forall uint256 i. i < ids.length => currentContract.caps[ids[i]].allocation + change <= 2 ^ 255 - 1, "assume updated allocation is small enough to cast to int256"; |
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.
same; explicit assumption is required and can not be proved assuming allocation<=max_int256
certora/specs/Reverts.spec
Outdated
| function currentContract.firstTotalAssets() external returns (uint256) envfree; | ||
| function currentContract.liquidityAdapter() external returns (address) envfree; | ||
| function currentContract.liquidityData() external returns (bytes) envfree; |
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.
| function currentContract.firstTotalAssets() external returns (uint256) envfree; | |
| function currentContract.liquidityAdapter() external returns (address) envfree; | |
| function currentContract.liquidityData() external returns (bytes) envfree; | |
| function firstTotalAssets() external returns (uint256) envfree; | |
| function liquidityAdapter() external returns (address) envfree; | |
| function liquidityData() external returns (bytes) envfree; |
| require ids[1] != ids[2], "ack"; | ||
|
|
||
| require forall uint256 i. i < ids.length => currentContract.caps[ids[i]].allocation > 0, "assume allocation is positive"; | ||
| require forall uint256 i. i < ids.length => currentContract.caps[ids[i]].allocation <= 2 ^ 255 - 1, "assume allocation is small enough to cast to int256"; |
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 one is proven in the rule allocationIsInt256 (same remark in summaryAllocate)
| assert !callerIsAllocator || !adapterIsRegistered || e.msg.value != 0 => lastReverted; | ||
| } | ||
|
|
||
| rule deallocateRevertCondition(env e, address adapter, bytes data, uint256 assets) { |
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.
do we need this rule here ? The file is named `*InputValidation', and we have a rule with the same name in the other specification file
| return (ids, change); | ||
| } | ||
|
|
||
| rule allocateInputValidation(env e, address adapter, bytes data, uint256 assets) { |
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.
Because we have the revert condition rule already, here we can just show that it reverts for the reasons that are not covered by the revert condition rule. More formally, if we have something like P => (Q <=> reverted) already, then we want to show that P => reverted and we can omit Q (because we know that P && Q => reverted.
More concretely, what is interesting is that if the allocation is 0 on some id then deallocate reverts, and if absolute cap is 0 on some id then allocate revert. This will prove that it's not possible to interact with unknown markets
| function libMulDivDown( | ||
| uint256 x, | ||
| uint256 y, | ||
| uint256 d | ||
| ) external pure returns (uint256) { | ||
| return MathLib.mulDivDown(x, y, d); | ||
| } |
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.
weird formatting: I get something else when doing forge fmt
| return (ids, change); | ||
| } | ||
|
|
||
| rule allocateRevertCondition(env e, address adapter, bytes data, uint256 assets) { |
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.
I think it would make sense to add all the assumptions that are made at the top of the rule, like for the sentinelCanDeallocate rules. This is because we are making a few assumptions, so it's not completely clear what this rule (and the next one) is proving
| require ids[1] != ids[2], "ack"; | ||
|
|
||
| // CVL does not allow function calls within quantifiers, hence explicitly listed here. | ||
| require currentContract.firstTotalAssets() * currentContract.caps[ids[0]].relativeCap <= max_uint256, "assume firstTotalASsets is bounded by max_uint256/WAD"; |
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.
| require currentContract.firstTotalAssets() * currentContract.caps[ids[0]].relativeCap <= max_uint256, "assume firstTotalASsets is bounded by max_uint256/WAD"; | |
| require currentContract.firstTotalAssets() * currentContract.caps[ids[0]].relativeCap <= max_uint256, "safe because firstTotalASsets cannot exceed by max_uint256/WAD"; |
I don't think this is really an assumption of the rule, because it is assumed that the loan token is passed to market v1, which bounds the total supply to max_uint128. Actually, the following would be even clearer:
| require currentContract.firstTotalAssets() * currentContract.caps[ids[0]].relativeCap <= max_uint256, "assume firstTotalASsets is bounded by max_uint256/WAD"; | |
| require currentContract.firstTotalAssets() <= 2 ^ 20 * 2 ^ 128, "market v1 fits total supply assets on 128 bits, and assume at most 2^20 markets"; |
| function summaryAllocate(env e, bytes data, uint256 assets, bytes4 selector, address sender) returns (bytes32[], int256) { | ||
| bytes32[] ids; | ||
| int256 change; | ||
| require ids.length == 3, "see IdsMorphoMarketV1Adapter"; |
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.
| require ids.length == 3, "see IdsMorphoMarketV1Adapter"; | |
| require ids.length == 3, "for simplicity, assume a fixed number of markets of 3"; |
because we cannot just assume that the adapter used is the market v1 adapter
| require ids[0] != ids[1], "ack"; | ||
| require ids[0] != ids[2], "ack"; | ||
| require ids[1] != ids[2], "ack"; |
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.
Let's give a reason for those: it's in the specs on adapters that they should return distinct ids
This PR proves RevertConditions for
allocate(),deallocate(),setName(),setSymbol(), andsetLiquidityAdapterAndData()Most of the constraints for
allocate()anddeallocate()are captured in the post-conditions of the adapter's allocate and deallocate.The revert conditions for the following remain pending due to certora issues: