Prevent CELO Transfer to Unreleased Treasury #11222

soloseng · 2024-09-24T00:07:50Z

Description

I propose this change to prevent sending funds back to the the CeloUnreleasedTreasury because the epochRewards contract uses the funds left in that contract to calculate the epoch rewards to be distributed.

This could potentially be used by malicious actors wanting to hide their identity, by registering as validator(elected) and/or voters, then sending stolen fund back to the CeloUnreleasedTreasury and receiving the stolen funds as rewards.

Although this scenario would distribute the funds to all validators and voters, depending on the amount it could be appealing.

In addition to that, any stolen funds received by legitimate validators could become a legal burden.

Tested

unit tests updated

Closes https://github.com/celo-org/celo-blockchain-planning/issues/643

no working test. Co-authored-by: Martín Volpe <[email protected]>

…o into feat/epoch-manager

martinvol · 2024-09-24T16:52:26Z

I'd add a fallback function and make it revert to make sure nobody overlooks this in the future

packages/protocol/contracts/common/GoldToken.sol

gitguardian · 2024-09-25T11:24:26Z

⚠️ GitGuardian has uncovered 3 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
10538986	Triggered	Generic High Entropy Secret	`efebfb7`	packages/protocol/scripts/foundry/constants.sh	View secret
10538986	Triggered	Generic High Entropy Secret	`04af0f7`	packages/protocol/scripts/foundry/constants.sh	View secret
10538986	Triggered	Generic High Entropy Secret	`29aadbb`	packages/protocol/scripts/foundry/constants.sh	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

…ransfer-to-unreleased-treasury

m-chrzan

Looks good overall, one comment about a missing test.

One concern: do we still have SELFDESTRUCT that will send CELO unconditionally? If so, this change is unfortunately somewhat moot.

packages/protocol/test-sol/unit/common/GoldToken.t.sol

soloseng · 2024-10-01T16:31:07Z

do we still have SELFDESTRUCT that will send CELO unconditionally? If so, this change is unfortunately somewhat moot

This is a valid point. Will have to think about mitigation for this case.

m-chrzan · 2024-10-03T09:07:54Z

This is a valid point. Will have to think about mitigation for this case.

If there is still SELFDESTRUCT, I think @pahor167's comment here becomes much more relevant - if this change doesn't fully provide the intended mitigation, might not be worth it to increase CELO ERC20 transfer costs.

Would a potential alternative solution be to keep account of the unreleased CELO within this contract, and simply ignore any balance transferred to it? I.e. for any calculations, rather than using this.balance, use a storage variable that gets decremented on release, and any CELO that gets transferred in accidentally/maliciously would become frozen (just as it would when transferring to any other core contract).

soloseng · 2024-10-03T14:27:00Z

Would a potential alternative solution be to keep account of the unreleased CELO within this contract

Yes, thats what I was thinking would be an alternate solution to this. We could then remove the CELOToken check to keep the ERC20 transfer gas low

openzeppelin-code · 2024-10-04T17:58:59Z

Prevent CELO Transfer to Unreleased Treasury

Generated at commit: c51610997ecc0236fa078b472fe126ff8c254845

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	3 2 0 14 43 62
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

…ransfer-to-unreleased-treasury

packages/protocol/contracts-0.8/common/CeloUnreleasedTreasury.sol

…ransfer-to-unreleased-treasury

martinvol · 2024-10-14T15:23:31Z

packages/protocol/contracts-0.8/common/CeloUnreleasedTreasury.sol

+      remainingBalanceToRelease = address(this).balance;
+      hasAlreadyReleased = true;
+    }
+
+    require(remainingBalanceToRelease >= amount, "Insufficient balance.");


I'd use ERC20 balance so that it can be ported to another token if needed.

Not sure I understand what you mean by porting to another token. As in distributing a different ERC20 token instead of CELO?

if we ever want to use this contract for another ERC20, it won't work at we use ERC20 interfaces instead of native ones.

martinvol · 2024-10-14T15:26:11Z

packages/protocol/contracts-0.8/common/CeloUnreleasedTreasury.sol


 import "../../contracts/common/Initializable.sol";
 import "./interfaces/ICeloUnreleasedTreasuryInitializer.sol";

 /**
 * @title Contract for unreleased Celo tokens.
+ * @notice This contract is not allowed to receive transfers of CELO,
+ * to avoid miscalculating the epoch rewards and to prevent any malicious actor
+ * from routing stolen fund through the epoch reward distribution.
 */


Can we make the fallback payable function explicitly revert?

We could add a fallback function to make it explicitly revert, but that would acheive the same result as it is currently.

martinvol · 2024-10-14T15:27:15Z

packages/protocol/test-sol/unit/common/GoldToken.t.sol

+    vm.prank(sender);
+    uint256 balanceBefore = celoToken.balanceOf(celoUnreleasedTreasuryAddress);
+
+    celoToken.transfer(celoUnreleasedTreasuryAddress, ONE_CELOTOKEN);
+    uint256 balanceAfter = celoToken.balanceOf(celoUnreleasedTreasuryAddress);
+    assertGt(balanceAfter, balanceBefore);


shouldn't this fail?

no, that's why we are now using the internal accounting. ERc20 transfers will succeeed, but we do not account for in in the release.

This is done to avoid checking the to address on every CELO token transfers.

See above comments 1 & 2 for more details

if ERC20 token transfer succeed, for consistency shouldn't we make native transfer for as well?

This follows the same pattern as all other core contracts. No fallback implemented but still receive ERC20 transfers. Since it now keeps an internal accounting of the remaining distribution, any transfers are ignored.

packages/protocol/contracts/common/GoldToken.sol

…ransfer-to-unreleased-treasury

soloseng and others added 30 commits August 15, 2024 17:08

first compiling draft

5f7e5d1

no working test. Co-authored-by: Martín Volpe <[email protected]>

Compiler fixes + finishNextEpochProcess

4084bac

allocateValidatorsRewards compilable

f3fed48

First few tests

0b473c2

test fix

b012f29

uisng registry instead

3a4f87a

updated registry ID var names

ad8831d

typo

5fa905d

++ mock contracts in 0.8

b3023c2

++ passing test using mock

9681185

Removal of using precompiles

18ba704

ScoreManager refactor

8d862d1

Decouple epoch manager initializer from registry

70b12a5

isReadyToStartEpoch fix

0c74a5e

startNextEpochProcess fixes

7379aff

rename score manager to score reader

7205bb7

Rename transfer to release on CeloDistribution schedule

b89cc74

Celo distribution schedule renamed to CeloUnreleasedTreasure

758d097

conditions to getFirstBlockAtEpoch and getLastBlockAtEpoch

d7b32fb

ScoreManager update

a66e578

getFirstBlockOfEpoch in EpochManager initializer

de3ae19

extra checks

8e86672

introduce new getValidatorsGroup

0fae892

systemAlreadyInitialized update

0e55b36

Removal of IEpochManagerInterface

973c21e

Made CI run on feature branch

315aaee

Merge branch 'release/core-contracts/12' into feat/epoch-manager

efebfb7

merge fix

4c9ee98

Added target for the CI

c6fd893

Merge branch 'feat/epoch-manager' of github.com:celo-org/celo-monorep…

04af0f7

…o into feat/epoch-manager

martinvol reviewed Sep 24, 2024

View reviewed changes

packages/protocol/contracts/common/GoldToken.sol Outdated Show resolved Hide resolved

++ comment

2a26cc6

Base automatically changed from feat/l2-epoch-system to release/core-contracts/12 September 25, 2024 11:24

martinvol requested a review from a team as a code owner September 25, 2024 11:24

martinvol requested a review from lvpeschke September 25, 2024 11:24

Merge branch 'release/core-contracts/12' into soloseng/prevent-celo-t…

63db861

…ransfer-to-unreleased-treasury

m-chrzan approved these changes Oct 1, 2024

View reviewed changes

packages/protocol/test-sol/unit/common/GoldToken.t.sol Outdated Show resolved Hide resolved

unused import

c516109

soloseng added 3 commits October 11, 2024 16:24

using internal accounting for released amount

19122fe

Merge branch 'release/core-contracts/12' into soloseng/prevent-celo-t…

5aafad1

…ransfer-to-unreleased-treasury

updated integration test

b182645

pahor167 reviewed Oct 14, 2024

View reviewed changes

packages/protocol/contracts-0.8/common/CeloUnreleasedTreasury.sol Show resolved Hide resolved

soloseng added 2 commits October 14, 2024 11:11

PR feedback

ec5ed87

Merge branch 'release/core-contracts/12' into soloseng/prevent-celo-t…

102b224

…ransfer-to-unreleased-treasury

martinvol reviewed Oct 14, 2024

View reviewed changes

packages/protocol/contracts/common/GoldToken.sol Show resolved Hide resolved

soloseng added 4 commits October 15, 2024 10:23

PR feedback

19c5138

Merge branch 'release/core-contracts/12' into soloseng/prevent-celo-t…

7778f3b

…ransfer-to-unreleased-treasury

reverted due to gas consideration

3d78151

more revert

539b513

soloseng merged commit 591b114 into release/core-contracts/12 Oct 15, 2024
39 of 42 checks passed

soloseng deleted the soloseng/prevent-celo-transfer-to-unreleased-treasury branch October 15, 2024 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent CELO Transfer to Unreleased Treasury #11222

Prevent CELO Transfer to Unreleased Treasury #11222

soloseng commented Sep 24, 2024 •

edited by martinvol

Loading

martinvol commented Sep 24, 2024

gitguardian bot commented Sep 25, 2024 •

edited

Loading

m-chrzan left a comment

soloseng commented Oct 1, 2024

m-chrzan commented Oct 3, 2024

soloseng commented Oct 3, 2024

openzeppelin-code bot commented Oct 4, 2024

martinvol Oct 14, 2024 •

edited

Loading

soloseng Oct 14, 2024 •

edited

Loading

martinvol Oct 15, 2024

martinvol Oct 14, 2024

soloseng Oct 14, 2024 •

edited

Loading

martinvol Oct 14, 2024

soloseng Oct 14, 2024 •

edited

Loading

martinvol Oct 15, 2024

soloseng Oct 15, 2024

Prevent CELO Transfer to Unreleased Treasury #11222

Prevent CELO Transfer to Unreleased Treasury #11222

Conversation

soloseng commented Sep 24, 2024 • edited by martinvol Loading

Description

Tested

martinvol commented Sep 24, 2024

gitguardian bot commented Sep 25, 2024 • edited Loading

⚠️ GitGuardian has uncovered 3 secrets following the scan of your pull request.

m-chrzan left a comment

Choose a reason for hiding this comment

soloseng commented Oct 1, 2024

m-chrzan commented Oct 3, 2024

soloseng commented Oct 3, 2024

openzeppelin-code bot commented Oct 4, 2024

Prevent CELO Transfer to Unreleased Treasury

🚨 Report Summary

martinvol Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

soloseng Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

martinvol Oct 15, 2024

Choose a reason for hiding this comment

martinvol Oct 14, 2024

Choose a reason for hiding this comment

soloseng Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

martinvol Oct 14, 2024

Choose a reason for hiding this comment

soloseng Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

martinvol Oct 15, 2024

Choose a reason for hiding this comment

soloseng Oct 15, 2024

Choose a reason for hiding this comment

soloseng commented Sep 24, 2024 •

edited by martinvol

Loading

gitguardian bot commented Sep 25, 2024 •

edited

Loading

martinvol Oct 14, 2024 •

edited

Loading

soloseng Oct 14, 2024 •

edited

Loading

soloseng Oct 14, 2024 •

edited

Loading

soloseng Oct 14, 2024 •

edited

Loading