docs: Add SphinxEscrow contract spec #1693
Conversation
RPate97
force-pushed
the
pate/escrow-spec
branch
2 times, most recently
from
a0c1c57
to
ee22fa0
Compare
RPate97
force-pushed
the
pate/escrow-spec
branch
from
ee22fa0
to
0ba74f1
Compare
| @@ -0,0 +1,60 @@ | |||
| # `SphinxEscrow` Contract Specification | |||
There was a problem hiding this comment.
General question: should this contract be deployed on every chain we support or a select number of chains?
|
|
||
| When deploying via the Sphinx DevOps Platform, the user has the option to pay for their deployments using ERC20 tokens on a single network. Before the deployment begins, the user must grant an allowance on an ERC20 token to the `SphinxEscrow` contract that is sufficient to cover the estimated cost of the deployment. The ERC20 token is then locked in the `SphinxEscrow` contract while the deployment is being executed. After the deployment is completed, the Sphinx backend calculates the exact cost of the deployment and transfers the remainder back to the user. | ||
|
|
||
| The Sphinx team owns the `SphinxEscrow` contract. |
There was a problem hiding this comment.
We should consider calling this contract something other than SphinxEscrow based on our conversation with the lawyer today
| ## Function-Level Invariants | ||
|
|
||
| #### `constructor(address _owner)` | ||
| - Must grant the `DEFAULT_ADMIN_ROLE` to the specified owner address. |
There was a problem hiding this comment.
It looks like we're using OpenZeppelin's AccessControl contract, but I only see the DEFAULT_ADMIN_ROLE used as a permission (in the disburse function). If we're planning on only having a single owner of this contract, we should use Ownable instead of AccessControl. If, instead, we're planning on allowing multiple addresses to withdraw funds, I think we should give them a specified role like we do in the ManagedService contract instead of giving them all the DEFAULT_ADMIN_ROLE. I think only one address should have the DEFAULT_ADMIN_ROLE because this address has permission to add or remove any other roles. This means that if we have multiple addresses with the DEFAULT_ADMIN_ROLE and we leak only one of their private keys, an attacker could remove all owners
| #### `function claim(address _owner, address _erc20, uint256 _amount)` | ||
| - Must revert if `_amount` is greater than the allowance of the `SphinxEscrow` contract on `_erc20` for `_owner`. | ||
| - Must revert if the `_owner` does not have a balance of `_erc20` that is >= `_amount`. | ||
| - A successful call must: | ||
| - Transfer the `_amount` of `_erc20` to the `SphinxEscrow` contract. | ||
| - Increment the `escrowBalance` of `_erc20` for `_owner` by `_amount`. | ||
| - Emit a `Claimed` event in the `SphinxEscrow` contract. | ||
|
|
||
| #### `function disburse(address _owner, address _feeRecipient, address _erc20, uint256 _refund, uint256 _cost)` |
There was a problem hiding this comment.
In both of these functions, the _owner parameter is the user, right? The semantics made me confused because the contract itself is owned by us
| - Must revert if `_amount` is greater than the allowance of the `SphinxEscrow` contract on `_erc20` for `_owner`. | ||
| - Must revert if the `_owner` does not have a balance of `_erc20` that is >= `_amount`. |
There was a problem hiding this comment.
Fyi these invariants may be enforced by the ERC20 standard itself, so it may be redundant to include these checks in our spec/contract. You don't need to worry about that though - I'll investigate this when I implement this contract
| - Emit a `Claimed` event in the `SphinxEscrow` contract. | ||
|
|
||
| #### `function disburse(address _owner, address _feeRecipient, address _erc20, uint256 _refund, uint256 _cost)` | ||
| - Must revert if the amount of `_erc20` currently in escrow for `_owner` is < `_refund` + `_cost` |
There was a problem hiding this comment.
- Must revert if the amount of
_erc20currently in escrow for_erc20is <_refund+_cost
This invariant allows for the possibility that only a fraction of the escrowed _erc20 is sent to the _owner or _feeRecipient. For example, if the escrowed _erc20 amount is 20 USDC, and _refund and _cost are both 5, then an error isn't thrown by this function. Is that intentional? I could see this being useful if the user wants to keep a balance of ERC20 for all deployments instead of adding a new allowance for each deployment. However, this use case isn't mentioned in the "Use Case" section, so it's not clear what the expected behavior is
| - Must revert if the `_owner` address is `address(0)`. | ||
|
|
||
| #### `function claim(address _owner, address _erc20, uint256 _amount)` | ||
| - Must revert if `_amount` is greater than the allowance of the `SphinxEscrow` contract on `_erc20` for `_owner`. |
There was a problem hiding this comment.
It looks like anybody can call this function to lock funds in the SphinxEscrow contract. This seems benign, but I'm not sure if it could mess up our backend logic. For example:
- Griefer grants the
SphinxEscrowcontract an allowance - Griefer approves deployment
- Griefer frontruns the
claimfunction, causing our call to theclaimfunction to throw an error
Unless there's a use case for keeping this open, I'd probably prefer to make it permissioned
Purpose
Adds the spec for the
SphinxEscrowcontract.