-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix DSU unwrapping in multi invoker and manager contract #461
Conversation
/// @param account Account to claim fees for | ||
/// @param unwrap Wheather to wrap/unwrap collateral on withdrawal | ||
function withdrawClaimable(address account, bool unwrap) external { | ||
if (msg.sender != account && !operators[account][msg.sender]) revert MultiInvokerUnauthorizedError(); |
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 since we use this pattern twice it might make sense to make it an onlyOperator
modifier or helper function like below in Manager.sol?
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.
done in f43bbf9
UFixed6 claimableAmount = claimable[account]; | ||
claimable[account] = UFixed6Lib.ZERO; | ||
|
||
_withdraw(account, claimableAmount, unwrap); |
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.
Our pattern has been to push funds to the msg.sender
rather than the account
(see: https://github.com/equilibria-xyz/perennial-v2/blob/v2.3/packages/perennial/contracts/Market.sol#L322)
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.
done in a6161d9
/// @notice withdraw DSU or unwrap DSU to withdraw USDC from this address to `account` | ||
/// @param account Account to claim fees for | ||
/// @param unwrap Wheather to wrap/unwrap collateral on withdrawal | ||
function withdrawClaimable(address account, bool unwrap) external { |
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.
nit: maybe just claim
?
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.
done in 329265f
@@ -137,6 +141,17 @@ abstract contract Manager is IManager, Kept { | |||
if (interfaceFeeCharged) emit TriggerOrderInterfaceFeeCharged(account, market, order.interfaceFee); | |||
} | |||
|
|||
/// @inheritdoc IManager | |||
function withdrawClaimable(address account, bool unwrap) external { |
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 comments on naming, where to send funds as above 👍
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.
UFixed6 claimAmount = market.claimFee(account); | ||
_withdraw(account, claimAmount, unwrap); | ||
claimable[account] = claimable[account].add(claimAmount); |
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.
Upon further thinking, we might just want to keep this as a atomic claim and withdraw rather than putting it in the claimable map.
Since these fees are already in DSU in the market, there's no real gain to claiming them into the multiinvoker and then withdrawing in a separate tx. The process of doing the claim+unwrap (if the unwrap is non-reverting) or a DSU claim (if the unwrap is reverting) is the same with or without the multiinvoker escrow
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.
Undid in a23a45e.
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 do not understand why MultiInvoker and Manager must hold funds to solve this problem. Seems a cleaner solution would be to use exception handling and push the DSU if it cannot be unwrapped.
/// @param unwrap Wheather to wrap/unwrap collateral on withdrawal | ||
function claim(address account, bool unwrap) external onlyOperator(account, msg.sender) { | ||
UFixed6 claimableAmount = claimable[account]; | ||
claimable[account] = UFixed6Lib.ZERO; |
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.
nit: delete claimable[account]
slightly cleaner here
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.
The mapping holds a UFixed6
, seemingly incompatible with the unary delete
operator.
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.
ope didn't know that 🙏
@@ -28,14 +28,12 @@ struct StoredTriggerOrder { | |||
/* slot 1 */ | |||
address interfaceFeeReceiver1; | |||
uint48 interfaceFeeAmount1; // <= 281m | |||
bool interfaceFeeUnwrap1; | |||
bytes5 __unallocated1__; | |||
bytes6 __unallocated1__; |
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.
nit: add a note that these contain dirty data until updated post v2.3 migration
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.
done in bb91a17
@@ -129,11 +125,9 @@ library TriggerOrderStorageLib { | |||
bytes6(0), | |||
newValue.interfaceFee1.receiver, | |||
uint48(UFixed6.unwrap(newValue.interfaceFee1.amount)), | |||
newValue.interfaceFee1.unwrap, | |||
bytes5(0), |
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.
should be bytes6
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.
corrected in bb91a17
bytes5(0), | ||
newValue.interfaceFee2.receiver, | ||
uint48(UFixed6.unwrap(newValue.interfaceFee2.amount)), | ||
newValue.interfaceFee2.unwrap, | ||
bytes5(0) |
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.
should be bytes6
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.
corrected in bb91a17
} | ||
|
||
/// @notice reads keeper compensation parameters from an action message | ||
function _compensateKeeperAction(Action calldata action) internal { |
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.
where are these two new function used? can't see them referenced in any of the other changes.
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.
Was a merge artifact; removed in bb91a17.
[Periphery] Unit Test Coverage ReportCoverage after merging prateek/pe-1740-fix-unwrapping into v2.3-fix-review will be
Coverage Report
|
[Core] Integration Test Coverage ReportCoverage after merging prateek/pe-1740-fix-unwrapping into v2.3-fix-review will be
Coverage Report
|
[Periphery] Integration Test Coverage ReportCoverage after merging prateek/pe-1740-fix-unwrapping into v2.3-fix-review will be
Coverage Report
|
[Periphery] Combined Test Coverage ReportCoverage after merging prateek/pe-1740-fix-unwrapping into v2.3-fix-review will be
Coverage Report
|
[Core] Unit Test Coverage ReportCoverage after merging prateek/pe-1740-fix-unwrapping into v2.3-fix-review will be
Coverage Report
|
[Core] Combined Test Coverage ReportCoverage after merging prateek/pe-1740-fix-unwrapping into v2.3-fix-review will be
Coverage Report
|
Issue: sherlock-audit/2024-08-perennial-v2-update-3-judging#16