Vault #220
Vault #220
Conversation
markspanbroek
force-pushed
the
update-solidity
branch
from
20a32c1 to
81e88dd
Compare
markspanbroek
force-pushed
the
update-solidity
branch
from
81e88dd to
b54cd59
Compare
|
Added functionality to have multiple accounts for an account holder inside a fund. This ensures that in the codex marketplace we keep funds for the client and for each slot separate, even though some providers may choose to fill multiple slots, or a client might decide to fill a slot in the storage contract itself. |
markspanbroek
force-pushed
the
vault
branch
2 times, most recently
from
3049325 to
724670b
Compare
|
Replaced 🔥 burnFund() with ❄️ freezeFund(). |
There was a problem hiding this comment.
Really great job Mark! Huge effort 💪 🙌
This was quite a lot to go through. I used the implementation branch (master...vault-integration#diff-5973808e32384782b5978a63e85b893b67d634be8aa42001f0ef480f189a0688R179) to get a better understanding of how some of the functionality relates to the Marketplace.
One question:
When we discussed freezing the vault, we discussed that it would not be able to be unfrozen by a controlling party. In this implementation, unfreezing happens after some time has elapsed, making the funds withdrawable again. Is the idea here that in the case of an attack, the vaults would all be locked in some way with a very long expiry?
| type Fund is bytes32; | ||
|
|
||
| /// Each fund has its own time lock | ||
| mapping(Controller => mapping(Fund => Lock)) private _locks; |
There was a problem hiding this comment.
Out of curiosity, why is there support for multiple Controllers? Do we anticipate having more than just the marketplace as the controller?
There was a problem hiding this comment.
I think this came from the idea that Vault should be "permanent", and hence, as we deploy new Marketplace contracts, the Vault will stay the same, and then you need to keep track of the "owner contract" of the funds who can manipulate them (eq. do transfers etc) as this is a standalone contract that potentially anybody could utilize.
That said, an alternative to this could be that every time we deploy a new Marketplace contract, we would also deploy a new Vault, and it would have a configurable single "owner" that would be set to the Marketplace contract.
There was a problem hiding this comment.
I see, or maybe it is to have one Vault deployed which allows for multiple, upgraded versions of the Marketplace contract.
| enum LockStatus { | ||
| /// Indicates that no lock is set. This is the initial state, or the state | ||
| /// after all tokens have been withdrawn. | ||
| NoLock, | ||
| /// Indicates that the fund is locked. Withdrawing tokens is not allowed. | ||
| Locked, | ||
| /// Indicates that the fund is frozen. Flows have stopped, nothing is allowed | ||
| /// until the fund unlocks. | ||
| Frozen, | ||
| /// Indicates that the lock is unlocked. Withdrawing is allowed. | ||
| Unlocked | ||
| } |
There was a problem hiding this comment.
Why do we need to distinguish between a NoLock state and an Unlocked state?
There was a problem hiding this comment.
I find these states a bit confusing, likely because I think of a lock as a binary dis/allowance. But here, we have 4 states that allow, or disallow, certain functions on the object being locked, and it becomes a bit cumbersome to try to remember which behaviours are allowed or disallowed for each state. I think maybe it would help me as a reader understand better if the names were a bit less abstract.
One option would be to rename the enum values with more descriptive names.
Another, more complex option, might be to use feature flags that describe the allowed functionality, almost like an ACL. A naive way would be to return an array of enum values for allowed lock behaviours but it's wasteful and requires iteration. A more efficient approach would be to return a uint8 value of bits, where a 1 at a bit index maps to an enum behaviour. So it could be something like the below. This idea removes the idea of freezing, and instead calls it "locking". And instead of the original idea of "locking", it is the concept of "freezing a lock" to "locking a lock" (where nothing can be done).
enum LockBehaviour {
Lock,
NewDeposits,
NewFlows,
SetExpiry,
SetMaximum,
Withdraw
}
/// A time-lock for funds
struct Lock {
// ...
/// Indicates whether fund is locked, and at what time
Timestamp lockedAt;
}
library Locks {
function allows(Lock memory lock, LockBehaviour behaviour) internal view returns (bool) {
uint8 allowed = lock.allowed();
// given behaviours 00000100, looking for Withdraw behaviour (2), shift to the right 2 places (00000001), then truncate to the least significant but (= 1)
return (allowed >> behaviour) & 1 == 1;
}
function allowed(Lock memory lock) internal view returns (uint8 allowed) {
if (Timestamps.currentTime() < lock.expiry) {
if (lock.lockedAt != Timestamp.wrap(0)) {
return 0; // no behaviours allowed
}
allowed = allowed + 1 << LockBehaviour.SetExpiry;
allowed = allowed + 1 << LockBehaviour.SetMaximum;
allowed = allowed + 1 << LockBehaviour.NewDeposits;
allowed = allowed + 1 << LockBehaviour.NewFlows;
allowed = allowed + 1 << LockBehaviour.Lock;
return allowed;
}
// was NoLock -- I'm still unsure as to why we need both this state and the "Unlocked" state
if (lock.maximum == Timestamp.wrap(0)) {
allowed = allowed + 1 << LockBehaviour.SetExpiry;
allowed = allowed + 1 << LockBehaviour.SetMaximum;
allowed = allowed + 1 << LockBehaviour.NewDeposits;
allowed = allowed + 1 << LockBehaviour.NewFlows;
allowed = allowed + 1 << LockBehaviour.Lock;
return allowed;
}
// was Unlocked
allowed = allowed + 1 << LockBehaviour.Withdraw;
return allowed; // i think not really needed since allowed is the "result var"
}
}Then later the usage for invariants/requires would be, eg:
function _withdraw(Controller controller, Fund fund, AccountId id) internal {
Lock memory lock = _locks[controller][fund];
require(lock.allows(LockBehaviour.Withdraw), WithdrawNotAllowed());
// ...
}
// originally "_lock"
function _update(
Controller controller,
Fund fund,
Timestamp expiry,
Timestamp maximum
) internal {
Lock memory lock = _locks[controller][fund];
require(lock.allows(LockBehaviour.SetExpiry, SetExpiryNotAllowed());
require(lock.allows(LockBehaviour.SetMaximum, SetMaximumNotAllowed());
lock.expiry = expiry;
lock.maximum = maximum;
_checkLockInvariant(lock);
_locks[controller][fund] = lock;
}
function _extendLock(
Controller controller,
Fund fund,
Timestamp expiry
) internal {
Lock memory lock = _locks[controller][fund];
require(lock.allows(LockBehaviour.SetExpiry, SetExpiryNotAllowed());
require(lock.expiry <= expiry, VaultInvalidExpiry());
lock.expiry = expiry;
_checkLockInvariant(lock);
_locks[controller][fund] = lock;
}Maybe a bit "too much" sorry 😅
There was a problem hiding this comment.
Why do we need to distinguish between a NoLock state and an Unlocked state?
I don't know, honestly, the current design/wording makes sense to me if you know how generally the Vault works. Eq. that you need to set the lock before you can deposit and manipulate funds and that for withdrawing, you have to have the lock "unlocked", which cannot be the same Enum state as otherwise, you could again "re-lock" the Funds.
But I agree with Eric that the wording here is a bit unfortunate. For me what I am a bit struggling with is the name for the LockStatus.Locked. In my mind, if you "lock" something, then you should not be able to manipulate it 😅 But here, you have to "lock" funds before you can deposit, transfer etc. In my mind, that does not really make sense.
There was a problem hiding this comment.
In my mind, if you "lock" something, then you should not be able to manipulate it 😅 But here, you have to "lock" funds before you can deposit, transfer etc.
Yes, I agree. And additionally, you can then freeze a lock that is locked 😅 That makes the concept of the lock quite confusing for me.
| LockStatus lockStatus = lock.status(); | ||
| if (lockStatus == LockStatus.Locked) { | ||
| Account memory account = _accounts[controller][fund][id]; | ||
| account.update(Timestamps.currentTime()); |
There was a problem hiding this comment.
I find update a bit awkward in the context of a view, where nothing is being stored, except for in memory. Maybe atEnd or simulateEndAt...?
There was a problem hiding this comment.
Yeah, I was also bit puzzled by this 😅 Maybe a better name would better convey what is happening here and why. I like the "simulate" word from Eric's suggestion, but then it does not make sense in the cases where the account is actually stored?
markspanbroek
force-pushed
the
update-solidity
branch
from
b54cd59 to
6816355
Compare
- no longer calculate flow updates when not needed - use account.update(timestamp) where needed - use _getBalance() to view current balance
- transfer ERC20 funds into the vault from the controller, not from the user - prevents an attacker from hijacking a user's ERC20 approval to move tokens into a part of the vault that is controlled by the attacker
There was a problem hiding this comment.
Great work Mark! 🎉
I don't see any big problem with the general approach and design here. My only concern is the usage of the uint128 for the balance tracking. See my comment bellow.
This is big PR, so this is just my first iteration of review. Will do more later on.
| /// controller. | ||
| /// | ||
| /// An account has a balance, of which a part can be designated. Designated | ||
| /// tokens can no longer be transfered to another account. |
There was a problem hiding this comment.
| /// tokens can no longer be transfered to another account. | |
| /// tokens can no longer be transferred to another account. Although they can be frozen/burned. |
|
|
||
| /// Returns the expiry time of the lock on the fund. A locked fund unlocks | ||
| /// automatically at this timestamp. | ||
| function getLockExpiry(Fund fund) public view returns (Timestamp) { |
There was a problem hiding this comment.
Should there be also a getter for the maximum lock?
|
|
||
| /// Locks the fund until the expiry timestamp. The lock expiry can be extended | ||
| /// later, but no more than the maximum timestamp. | ||
| function lock( |
There was a problem hiding this comment.
What about an override that does not have the maximum? Eq. the maximum would equal to expiry.
| struct Balance { | ||
| /// Available tokens can be transfered | ||
| uint128 available; | ||
| /// Designated tokens can no longer be transfered | ||
| uint128 designated; | ||
| } |
There was a problem hiding this comment.
I am not sure about the uint128. I understand your reasoning, but then if this should be a "general purpose contract", doing this optimization is IMHO wrong. You also don't have balances in ERC20 tracked with uint128 even though "it should be good enough for most cases".
There was a problem hiding this comment.
Looking at how this propagates to the Marketplace contract, I am more unsure about this.
| /// | ||
| /// The vault maintains a number of invariants to ensure its integrity. | ||
| /// | ||
| /// The lock invariant ensures that there is a maximum time that a fund can be |
There was a problem hiding this comment.
| /// The lock invariant ensures that there is a maximum time that a fund can be | |
| /// The _checkLockInvariant() invariant ensures that there is a maximum time that a fund can be |
| /// lock.expiry <= lock.maximum | ||
| /// where lock = _locks[controller][fund]) | ||
| /// | ||
| /// The account invariant ensures that the outgoing token flow can be sustained |
There was a problem hiding this comment.
| /// The account invariant ensures that the outgoing token flow can be sustained | |
| /// The _checkAccountInvariant() invariant ensures that the outgoing token flow can be sustained |
| type Fund is bytes32; | ||
|
|
||
| /// Each fund has its own time lock | ||
| mapping(Controller => mapping(Fund => Lock)) private _locks; |
There was a problem hiding this comment.
I think this came from the idea that Vault should be "permanent", and hence, as we deploy new Marketplace contracts, the Vault will stay the same, and then you need to keep track of the "owner contract" of the funds who can manipulate them (eq. do transfers etc) as this is a standalone contract that potentially anybody could utilize.
That said, an alternative to this could be that every time we deploy a new Marketplace contract, we would also deploy a new Vault, and it would have a configurable single "owner" that would be set to the Marketplace contract.
| LockStatus lockStatus = lock.status(); | ||
| if (lockStatus == LockStatus.Locked) { | ||
| Account memory account = _accounts[controller][fund][id]; | ||
| account.update(Timestamps.currentTime()); |
There was a problem hiding this comment.
Yeah, I was also bit puzzled by this 😅 Maybe a better name would better convey what is happening here and why. I like the "simulate" word from Eric's suggestion, but then it does not make sense in the cases where the account is actually stored?
| enum LockStatus { | ||
| /// Indicates that no lock is set. This is the initial state, or the state | ||
| /// after all tokens have been withdrawn. | ||
| NoLock, | ||
| /// Indicates that the fund is locked. Withdrawing tokens is not allowed. | ||
| Locked, | ||
| /// Indicates that the fund is frozen. Flows have stopped, nothing is allowed | ||
| /// until the fund unlocks. | ||
| Frozen, | ||
| /// Indicates that the lock is unlocked. Withdrawing is allowed. | ||
| Unlocked | ||
| } |
There was a problem hiding this comment.
Why do we need to distinguish between a NoLock state and an Unlocked state?
I don't know, honestly, the current design/wording makes sense to me if you know how generally the Vault works. Eq. that you need to set the lock before you can deposit and manipulate funds and that for withdrawing, you have to have the lock "unlocked", which cannot be the same Enum state as otherwise, you could again "re-lock" the Funds.
But I agree with Eric that the wording here is a bit unfortunate. For me what I am a bit struggling with is the name for the LockStatus.Locked. In my mind, if you "lock" something, then you should not be able to manipulate it 😅 But here, you have to "lock" funds before you can deposit, transfer etc. In my mind, that does not really make sense.
Adds a
Vaultcontract, that allows funds to be locked up for a certain amount of time. These funds can be transferred, burned or even flow over time to other addresses.Requires #219 to be merged first.