-
Notifications
You must be signed in to change notification settings - Fork 18
refactor: addressing internal audit #216
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
Changes from 13 commits
da3d8e9
2c4d96d
fd4aea0
cd5a0a9
e38d964
1555832
f36648a
a71476d
87bac92
c1ba5f6
96621e5
ceb931c
d4de5d2
adc82db
368b741
dec034b
513a2fc
1e272d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -140,7 +140,7 @@ abstract contract DefaultTargetDispenserL2 is IBridgeErrors { | |
|
|
||
| /// @dev Processes the data received from L1. | ||
| /// @param data Bytes message data sent from L1. | ||
| function _processData(bytes memory data) internal { | ||
| function _processData(bytes memory data) internal returns (uint256 totalAmount) { | ||
| // Reentrancy guard | ||
| if (_locked > 1) { | ||
| revert ReentrancyGuard(); | ||
|
|
@@ -198,6 +198,9 @@ abstract contract DefaultTargetDispenserL2 is IBridgeErrors { | |
| emit AmountWithheld(target, targetWithheldAmount); | ||
| } | ||
|
|
||
| // Update total to-be-deposited amount | ||
| totalAmount += amount; | ||
|
Comment on lines
+212
to
+213
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add up to the total amount to-be-deposited |
||
|
|
||
| uint256 olasBalance = IToken(olas).balanceOf(address(this)); | ||
| // Check the OLAS balance and the contract being unpaused | ||
| if (olasBalance >= amount && localPaused == 1) { | ||
|
|
@@ -329,14 +332,31 @@ abstract contract DefaultTargetDispenserL2 is IBridgeErrors { | |
| /// - Token transfer succeeds, message fails: call this function; | ||
| /// - Token transfer fails, message succeeds: re-send OLAS to the contract (separate vote). | ||
| /// @param data Bytes message data that was not delivered from L1. | ||
| function processDataMaintenance(bytes memory data) external { | ||
| /// @param updateWithheldAmount True, if withheld amount update is required. | ||
| function processDataMaintenance(bytes memory data, bool updateWithheldAmount) external { | ||
| // Check for the contract ownership | ||
| if (msg.sender != owner) { | ||
| revert OwnerOnly(msg.sender, owner); | ||
| } | ||
|
|
||
| // Process the data | ||
| _processData(data); | ||
| // Process the data and calculate deposited amounts | ||
| uint256 totalAmount = _processData(data); | ||
|
|
||
| // Update withheld amount | ||
| if (updateWithheldAmount) { | ||
| uint256 localWithheldAmount = withheldAmount; | ||
|
|
||
| // Check for overflow | ||
| if (totalAmount > localWithheldAmount) { | ||
| revert Overflow(totalAmount, localWithheldAmount); | ||
| } | ||
|
|
||
| // Update withheld amount | ||
| localWithheldAmount -= totalAmount; | ||
| withheldAmount = localWithheldAmount; | ||
|
|
||
| emit WithheldAmountUpdated(localWithheldAmount); | ||
|
Comment on lines
+342
to
+369
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calculate total amount being deposited from the balance of this contract. If the balance is used that is recorded in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure why there should be an explicit calculation here, we had this implemented in such a way the DAO needs to take care to add correct numbers. Let's sync tomorrow on this
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I approve this, since we discussed and converged on this approach |
||
| } | ||
|
|
||
| emit StakingMaintenanceDataProcessed(data); | ||
| } | ||
|
|
@@ -350,6 +370,11 @@ abstract contract DefaultTargetDispenserL2 is IBridgeErrors { | |
| } | ||
| _locked = 2; | ||
|
|
||
| // Check for the contract ownership | ||
| if (msg.sender != owner) { | ||
| revert OwnerOnly(msg.sender, owner); | ||
| } | ||
|
Comment on lines
+384
to
+387
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding owner check as suggested by the audit. |
||
|
|
||
| // Pause check | ||
| if (paused == 2) { | ||
| revert Paused(); | ||
|
|
@@ -401,9 +426,13 @@ abstract contract DefaultTargetDispenserL2 is IBridgeErrors { | |
| _locked = 1; | ||
| } | ||
|
|
||
| /// @dev Updates withheld amount manually by the DAO in order to account for `processDataMaintenance()` amounts. | ||
| /// @notice The amount here must correspond to the exact withheldAmount minus the accumulation of all the previous | ||
| /// unique amounts deposited via `processDataMaintenance()` function execution. | ||
| /// @dev Updates withheld amount manually by the DAO in order to: | ||
| /// [1] Account for not recorded `processDataMaintenance()` amounts; | ||
| /// [2] Withheld amount update after balance migration to a new contract. | ||
| /// @notice The amount here must correspond to: | ||
| /// [1] The exact withheldAmount minus the accumulation of all the previous | ||
| /// unique amounts deposited via `processDataMaintenance()` function execution; | ||
| /// [2] Final OLAS balance of this contract address. | ||
| /// @param amount Updated withheld amount. | ||
| function updateWithheldAmountMaintenance(uint256 amount) external { | ||
| // Check the contract ownership | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| # Tokenomics Flowchart | ||
|
|
||
| ```mermaid | ||
| graph TD | ||
| %% Tokenomics | ||
| subgraph tokenomics [Tokenomics] | ||
| Treasury[Treasury] | ||
| Dispenser[Dispenser] | ||
| DonatorBlacklist[DonatorBlacklist] | ||
| Tokenomics[Tokenomics] | ||
| Depository[Depository] | ||
| GenericBondCalculator[Generic Bond Calculator] | ||
| DepositProcessorL1[DepositProcessorL1] | ||
| TargetDispenserL2[TargetDispenserL2] | ||
| end | ||
|
|
||
| subgraph governance [Governance] | ||
| OLAS_Token[OLAS Token] | ||
| Timelock@{ shape: div-rect, label: "Timelock" } | ||
| veOLAS[veOLAS] | ||
| end | ||
|
|
||
| subgraph registries [Registries] | ||
| AgentRegistry[Agent and Component Registry] | ||
| ServiceRegistry[Service Registry] | ||
| StakingProxy[StakingProxy] | ||
| end | ||
|
|
||
| LP_Token[LP Token] | ||
| Owner([OLAS or LP Token owner]) | ||
| OwnerAgent[[Component or Agent Owner]] | ||
| AnyWallet([Any Wallet or Contract]) | ||
|
|
||
| AnyWallet-->|depositServiceDonationETH|Treasury | ||
| DepositProcessorL1==>|bridge: tokens, message|TargetDispenserL2 | ||
| Depository-->|calculatePayoutOLAS|GenericBondCalculator | ||
| Depository-->|reserveAmountForBondProgram, refundFromBondProgram|Tokenomics | ||
| Depository-->|depositTokenForOLAS|Treasury | ||
| Depository-->|transfer|OLAS_Token | ||
| Dispenser-->|claimOwnerIncentives, claimStakingIncentives|Tokenomics | ||
| Dispenser-->|sendMessage|DepositProcessorL1 | ||
| Dispenser-->|withdrawToAccount|Treasury | ||
| GenericBondCalculator-->|getLastIDF|Tokenomics | ||
| Owner-->|deposit, redeem|Depository | ||
| OwnerAgent-->|claimOwnerRewards|Dispenser | ||
| TargetDispenserL2-->|deposit|StakingProxy | ||
| Timelock-->|changeOwner|Dispenser | ||
| Timelock-->|changeOwner|Tokenomics | ||
| Timelock-->|changeOwner, create, close|Depository | ||
| Timelock-->|changeOwner, withdraw, enableToken, disableToken|Treasury | ||
| Treasury<-->|trackServiceDonation, rebalanceTreasury|Tokenomics | ||
| Tokenomics-->|ownerOf, totalSupply|AgentRegistry | ||
| Tokenomics-->|getComponentIdsOfServiceId, getAgentIdsOfServiceId|ServiceRegistry | ||
| Tokenomics-->|inflationRemainder, totalSupply|OLAS_Token | ||
| Tokenomics-->|getVotes|veOLAS | ||
| Tokenomics-->|isDonatorBlacklisted|DonatorBlacklist | ||
| Treasury-->|drain|ServiceRegistry | ||
| Treasury-->|transferFrom|LP_Token | ||
| Treasury-->|mint, transfer|OLAS_Token | ||
| ``` |
Uh oh!
There was an error while loading. Please reload this page.
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.
Since proposed deposited amounts could be further cut off by the
StakingFactoryverification and result in morewithheldAmount-s, let's compute the total amount that is deposited.