[AUDIT] Feature: CrossChain PP Base & Everclear CrossChain PP #737
Conversation
* add cross-chain module, contracts compile * refactor tempalte module * fix compile issues * add todo's for implementation contract * added notes handoff * add more notes * fix compile issues before everclear impl * add everclear poc impl * add everclear logic impl * remove old templates * code compile issue solved * code compile issue solved * fix minor dependency * remove file * added contract for bridge testing * comment our bridge test * add tests for base contracts * fix file structure * uncomment bridge return value and run fmt * change contract to abstract * add logic to PP_Connext_Crosschain * fix inheritance chain * remove function impl * restructure directories * seperate interfaces * remove unecessary state variables * add templates * contracts compile * adjust issues from inheritance chain * readd execute Bridge * fix compile error --------- Co-authored-by: Zuhaib Mohammed <[email protected]> Co-authored-by: Marvin <[email protected]> Co-authored-by: Zitzak <[email protected]>
…terNetwork/inverter-contracts into audit-payment-order-struct
* add cross-chain module, contracts compile * refactor tempalte module * fix compile issues * add todo's for implementation contract * added notes handoff add more notes * fix compile issues before everclear impl * add everclear poc impl * add everclear logic impl * remove old templates * code compile issue solved code compile issue solved * fix minor dependency * remove file * added contract for bridge testing comment our bridge test * add tests for base contracts * fix file structure * uncomment bridge return value and run fmt * change contract to abstract * add logic to PP_Connext_Crosschain * fix inheritance chain * remove function impl * seperate interfaces * remove unecessary state variables * remove unecessary state variables * add unit tests for Connext * unit test processPayments & xcall fix merge conflicts fix file structure fix merge conflicts finish rebasing * fix directory structure * unit test chainid test * unit test processpayments test * unit test paymentId check and test * unit test chainId and ProcessPayment * unit test paymentId check and test * resolve conflicts rebasing rebasing * add unit test verify bridgeData * add unit test process payments insufficient balance * add feedback comment fix paths after rebase rebased and add init * fixed unit tests after rebase remove init functions * add comments and fuzz tests add notes * refrator test file and add unit tests * verify executionData & add unit tests * add ttl validation * refractor unit tests * add unit tests CrosschainBase_v1 * singlePayment fuzz tests added * move balance setup internal function * multiplePayment fuzz tests added * camelcase changes for CrossChainBase contract * add gherking comments * remove unecessary variables and add comments * add more gherkin to each test * add validation of ttl * add getbridgedata and natspec * chore: cleaned code changed event name * add templates folder * emit PaymentProcessed via Interface * chore: use exposed payment processor * update gherkin for tests * add assertions to payment processor test * update gherkin and remove vs code settings * feat: add cancel payments * feat:add cancel payment & test * feat:add retry payments & test * fix:retry payments test * fix:format * fix:fmt and standard updates * chore:delete bridging folder * fix:rebase onto feature * chore:remove redundant files * fix:add standard to test file * chore:remove redundant files * fix:update crosschain base format * fix:delete console import * fix:move files to proper directories * fix:remove impl for functions * fix: fix retry failed payments * chore:add comments for clarity * fix:remove files * add tests for unhappy paths * chore:change mapping name * remove redundant file * chore:rename file * add base test file * fix: remove chainid impl * chore: add unit tests * chore: add unit tests * fix: make unit tests generic input * fix: process payment if else * chore: add unit tests for PP_Crosschain_v1 * fix: add auth checks and retrypayment issue and format unit tests * fix: change test to fuzz tests, fix minor bugs * fix: code format invertor standard * fix: change the _validateTransferRequest msg.sender to client * fix: refractor the unit tests * chore: add unit tests for client.amountPaid functionality * fix: track processedIntentId fix * feat: add unclaimed amount impl * fix: format code * fix: follow invertor standard * refactor: Rename ERC20PaymentClientBase_v1 to v2 * refactor: adapt implementation and tests to new Payment order struct What has been done: - Comment implementation where it didn't compile instead of updating it - Copied and commented test that were failing because of the change. Changed the tests to work with empty data * feat(IERC20PaymentClientBase_v2): add maxFee and TTL to payment order comment * refactor: CrossChainBase_v1 & ICrossChainBase_v1 What has been done: - Removed `processPayments()` as the base is no PP - Remove `executionData` from `executeBridgeTransfer` because of move to new payment order - Add __gap for upgrade - Removed unnused events/errors/struct - Inverter Standard - Cleaned up import - Update headers in files - Moved functions to the correct place in file - Append underscore in function parameters - Add explicit name to return variables * refactor: PP_Crosschain_v1 & IPP_Crosschain_v1 What has been done? - Remove `processPayments()` as it's not an implementation but abstract - Moved `validPaymentOrder()` from this abstract to the connext implementation, because what makes a payment order valid differs for each implementation given the data/flags field send - Moved the following functionalities from the connext implementation to this abstract, as each crosschain PP will need them, so they can be in the base: - `unclaimable()` - `claimPreviouslyUnclaimable()` - the internal mapping to track the unclaimable amounts - Removed internal functions `validTimes()` as the crosschain PP doesn't vest orders - Removed unused events/errors/structs - Inverter Standard - Made state variable internal and add getters - Cleaned up import - Update headers in files - Moved functions to the correct place in file - Append underscore in function parameters - Add explicit name to return variables * refactor: PP_Connext_Crosschain_v1 & IPP_Connext_Crosschain_v1 What has been done: - Update contract with the new payment order struct, which introduces the following changed: - Add Flags constants that are used to validate and read the data field of the payment order - Add internal functions `_validateFlagsAndData()` & `_getEverclearMaxFeeAndTTL()` to validate and retrieve the `maxFee` and `ttl` from the data field of the payment order - Remove all parameters for `executionData` as the values needed are now retrieved from the data fields of the payment order struct - Moved `unclaimableAmountsForRecipient` state variable from this implementation to the base abstract contract - Add internal functions `_validPaymentOrder()` which checks all the values of the new payment order. Also add call to this function in `processPayments()` - Add internal function `_transferTokenAndApproveToBridge()` which is called when we pull assets from the payment client, but not when retrying bridging, removing the need to pass as boolean param to internal function `_createCrossChainIntent()` - Renamed `retryFailedTransfer()` to `retryFailedBridgeTransfer()` to be more explicit - Removed unused events/errors/struct - Inverter Standard - Cleaned up import - Update headers in files - Moved functions to the correct place in file - Make state variable internal and add getters - Append underscore in function parameters - Add explicit name to return variables * test: WIP * fix: code refactor & fix unit tests * fix: code refactor & fix unit tests * chore: add interface * chore: inverter standard changes #1 * chore: inverter standard changes #2 * chore: standardize CrossChain naming convention across files and contracts, and remove unused imports in tests * fix:retunr data from mapping * fix:compliation error * refactor: refactor natspec and state variable visibility What has been done? - Add `.` after comments - Set constants back to public after mistake to make them private - update author * refactor: Update natspec * refactor: clean up test file, remove duplicate file after renaming * refactor: update version in mocks * Fix camelcase naming issue * Remove case-conflicting file from remote tracking * Delete test/modules/paymentProcessor/abstracts/CrosschainBase_v1.t.sol Delete duplicate file * fix: resolve renaming issue * fix: inverter standard and event emission * fix:missing modifier * fix:fix failing tests * fix:fix gherkin in base test * fix:fix tag spacing and inverter standard comments * fix:fix imports in tests * fix:remove helper function * fix:tag spacing * fix:camel case contract names * fix: fix tests add fuzzing * fix: function params * fix:init natsec * fix: folder for mocks * fix: camelcase file issue * fix:camel casing and compliation * fix: file thingy issue * Remove camelCase * fix:compliation erro * fix:test names * refactor: Delete CrossChainBase_v1 contract and interface Why? - The contract is supposed to function as base crossChain contract for all modules. However it is leaning to much towards payment processing due to the use of payment orders. Because of that, the decision is make to merge it with the PP_CrossChain contract * refactor: PP_CrossChain_v1 What has been done? - Add deleted functionalities from CrossChainBase - Update contract Natspec - Update event that emits bridge intend - Add more members to make it easier for the indexer - rename event - Add missing Inverter Standard changes * refactor: PP_Connext_CrossChain_v1 What has been done? - Update contract natspec - Update event when bridging has been succesful - Add missing changes for Inverter Standard * refactor: Rename implementation contract to PP_Everclear_CrossChain_v1 Why? The contract was named after the bridging infra we used initially, i.e. Connext. This has been changed to Everclear * refactor: rename PP_CrossChain into PP_CrossChainBase Why? As this is the base crosschain contract for all the payment processors * test: add PP_CrossChainBase test file * refactor: PP_Everclear_CrossChain test * fix:fix compliation errors * fix:tests and revert statements * chore:format * fix:add test cases for exposed function * refactor: PP_CrossChainBase & PP_EverClear_CrossCahin What has been done? - Add TokenReleased event - Removed unused parameters - Removed unused WETH - Inverter standard * wip * fix:add tests for execute bridge transfer * fix:add test for transfer token function * fix:add valid payment order test * chore:fmt * bugfix: resolve test failing in PP_Everclear_CrossChain --------- Co-authored-by: Zuhaib Mohammed <[email protected]> Co-authored-by: Zitzak <[email protected]> Co-authored-by: JeffreyJoel <[email protected]> Co-authored-by: Marvin <[email protected]>
| // Process each payment order. | ||
| for (uint i = 0; i < orders.length; i++) { | ||
| if (!_validPaymentOrder(orders[i])) { | ||
| revert |
There was a problem hiding this comment.
It could be safer to skip invalid orders instead of reverting, as revet will get the system essentially stuck, which would be a risk in case users are allowed to add payment orders in a way which is not fully validated
There was a problem hiding this comment.
In general the required dependency that all orders must be successful is somewhat risky to get payments stuck for everyone
There was a problem hiding this comment.
I believe you brought this up with the other payment processor as well and it's a valid point. However when adding a payment order through a payment client, calling the function _addPaymentOrder(), it will always call the Payment processor's public validPaymentOrder() function, ensuring that no invalid order can be added. Because of that reason we have the revert, as it would point to the fact that more is going wrong then just failed order. Hope that made sense @omega-audits
There was a problem hiding this comment.
@FHieser happy to have your input here as well
There was a problem hiding this comment.
In addition to what @Zitzak said:
Every processPayments() call is currently preceded by the according addPaymentOrder call in the same transaction, which ensures that the PaymentOrders must be valid. In that sense we actually do the check multiple times, which definitely isnt ideal. Reverting seems correct here, as it currently would prevent the orginal faulty PaymentClient function to run through (faulty in the sense that it produced a invalid paymentOrder in the first place).
For context: The validPaymentOrder check was a later addition to the payment system and could have probably be handled a bit better. As it currently stands this coding except for costing some gas is not harmful in any way, I would (assuming my statement form above stands, @Zitzak I currently dont know if the new Oracle Fundingmanager changes that) say we just leave it as is and bring this up internally, if we can do something about the double validation.
src/modules/paymentProcessor/abstracts/PP_CrossChainBase_v1.sol
Outdated
Show resolved
Hide resolved
| } | ||
| // Transfer the token for the order from the payment client into | ||
| // the payment processor. | ||
| _transferTokenAndApproveToBridge(orders[i], address(client_)); |
There was a problem hiding this comment.
It could make sense to do this action of transfer and approve of the tokens for all orders combined (unless you fully remove the potential atomic failures in the action)
There was a problem hiding this comment.
As my current choice would be to keep the revert for invalid order, I believe we indeed need to keep the single approval/transfer. Wdyt?
There was a problem hiding this comment.
But if you wan to revert in case any of the orders fail, that is actually a better reason to do the combined approved/ transfer for all of them at the end instead of for each order no?
There was a problem hiding this comment.
@Zitzak and I had a talk about this. This comment is more of FYI
This contract is essentially a PaymentProcessor Base contract (like the BondingCurveBase)
With some small additions (paymentIdToBridge etc)
I think it makes sense to finally abstract the Base into its own contract, which we will Hopefully tackle in the future.
Tagging @0xNuggan and @marvinkruse for visibility
| delete _unclaimableAmountsForRecipient[client_][token_][sender]; | ||
|
|
||
| IERC20(token_).transfer(paymentReceiver_, amount); | ||
| emit TokensReleased(paymentReceiver_, address(token_), amount); |
There was a problem hiding this comment.
may want to add the msg.sender in the event - it seems important to know who's tokens were claimed
There was a problem hiding this comment.
As this is a Payment Process event, the change here would effect quite a lot of code. I do agree however with your point. @FHieser any thoughts here?
There was a problem hiding this comment.
This is essentially an extension of the amountPaid doesnt have enough info/open redemption amount from Queued PP here.
As we still dont know how extensively the claimUnclaimableAmount functioanlity will be used and we decided already to tackle this (we tracked this internally via Issue) I think it should be fine if we leave it as is for now and go for it later. Just add this to the created issue as a reference point and we are good to go.
What has been done? - add safe transfer - deleted retryFailedBridgeTransfer function - moved storage slot gap
What has been done? - add safe transfer - move gap
Audit Scope
This PR introduces two new contracts to the
devbranch for which an Audit is requested. Those two contracts are as following:Both contracts have unit tests