Post contest updates #57
Conversation
sunbreak1211
changed the title
sunbreak1211
requested review from
oldchili and
telome
and removed request for
oldchili
| function isUrnAuth(address urn, address usr) external view returns (bool ok) { | ||
| ok = _urnAuth(urn, usr); | ||
| function isUrnAuth(address owner, uint256 index, address usr) external view returns (bool ok) { | ||
| ok = _urnAuth(owner, _getUrn(owner, index), usr); | ||
| } | ||
|
|
||
| // --- urn management functions --- | ||
|
|
||
| function open(uint256 index) external returns (address urn) { |
There was a problem hiding this comment.
-
Technically you don't need the
indexinput, and can return it to the user along with theurn.
The docs says it is there to prevent race condition, but by the same logic every user operation should be with a nonce to prevent him from, e.g., callinglocktwice.
Also, the consequences of a race condition in open are not so bad, as the user still owns both urns. -
I wonder if it make sense to let anyone open a
urnon behalf of a user. and in this case, the function won't revert if the index already exist, and instead will just return theurn.
There was a problem hiding this comment.
Don't forget that we support multicall.
So for (1) - The user might not remember how many urns he has and passing the index can avoid mistakes such as opening urn number 3 but depositing to urn number 2.
For (2) - Also here I see cases for confusion since someone might increment the count for the user without him being aware, which may end up locking in the wrong urn. And if the index is still used for validation it can become a DOS vector on opens. In general I think this is also the type of change we'd prefer not to do at this point after the Sherlock audit.
| * `wipe(address urn, uint256 wad)` - Repay `wad` amount of NST backed by the `urn`’s MKR. | ||
| * `wipeAll(address urn)` - Repay the amount of NST that is needed to wipe the `urn`s entire debt. | ||
| * `getReward(address urn, address farm, address to)` - Claim the reward generated from a farm on behalf of the `urn` and send it to the specified `to` address. | ||
| * `hope(address owner, uint256 index, address usr)` - Allow `usr` to also manage the `owner-index` `urn`. |
There was a problem hiding this comment.
it is only a matter of semantic, but maybe a name like deployer is better than owner.
It is true that currently the deployer has owner privileges in the _urnAuth function, but imo, it is more of a bug than a feature (though legit design decision).
There was a problem hiding this comment.
deployer has usually other meaning in Maker scripts (the deployer of the main contract).
| mapping(address owner => uint256 count) public ownerUrnsCount; | ||
| mapping(address owner => mapping(uint256 index => address urn)) public ownerUrns; | ||
| mapping(address urn => address owner) public urnOwners; | ||
| mapping(address urn => mapping(address usr => uint256 allowed)) public urnCan; |
There was a problem hiding this comment.
not super important, but i wonder if a mapping from owner and index to allowed (and similarly to voteDelegate, farm, auctionsCount etc) would not be more consistent.
Possibly even easier for UI integration, though i guess the UI has to be aware of the urn to query for the user debt/collateral position.
There was a problem hiding this comment.
I have originally made the can mapping in this PR like that way, but then I preferred to make all the mappings under urn to keep some consistency. I agree we could have moved everything to owner/index as well, but that probably ended up being a bigger change and in general the urn as index still makes sense.
| function isUrnAuth(address urn, address usr) external view returns (bool ok) { | ||
| ok = _urnAuth(urn, usr); | ||
| function isUrnAuth(address owner, uint256 index, address usr) external view returns (bool ok) { | ||
| ok = _urnAuth(owner, _getUrn(owner, index), usr); |
There was a problem hiding this comment.
- is there a reason to revert if
(owner,index)does not have aurnand in this case_getUrnwill revert? it could be annoying for a UI if a view function revert. - if
_getUrnis not used, i wonder if you can't just make this function public and get rid of_urnAuth, but i guess it is nicer to have it the way it is now.
There was a problem hiding this comment.
Checking the urn existence is important for the lock functions in order to avoid to deposit something in a position that doesn't exist. In the prev version, validating the urn existence would require an extra storage load, so it was just implemented in those lock functions. Now that we are already consuming the extra sload, IMO just made sense to put it in the checks path everywhere.
I doubted about the consequence of reverting in isUrnAuth as well. If we consider this might be a pain in the ass for the UIs we can do it differently there.
|
|
||
| function _getUrn(address owner, uint256 index) internal view returns (address urn) { | ||
| urn = ownerUrns[owner][index]; | ||
| require(urn != address(0), "LockstakeEngine/invalid-urn"); |
There was a problem hiding this comment.
just curious, if you remove the check for urn != address(0) will something break?
It is probably good to have it anw, I just want to make sure i don't miss anything in the threat model.
There was a problem hiding this comment.
As mentioned in the prev comment, not really breaking, but it is a nice check when locking. And in general it kinda feels good to first check something is valid before checking its permissions, but yeah far from being mandatory or similar.
|
|
No description provided.