Platform: Code4rena
Start Date: 22/09/2022
Pot Size: $30,000 USDC
Total HM: 12
Participants: 133
Period: 3 days
Judge: 0xean
Total Solo HM: 2
Id: 165
League: ETH
Rank: 7/133
Findings: 5
Award: $1,134.47
π Selected for report: 2
π Solo Findings: 0
π Selected for report: ladboy233
Also found by: 0xSmartContract, 8olidity, Aymen0909, Chom, IllIllI, OptimismSec, PaludoX0, TomJ, ayeslick, cccz, csanuragjain, joestakey, neko_nyaa, pashov, peritoflores, rbserver, rvierdiiev
19.982 USDC - $19.98
https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/ERC20/ERC20PermitPermissionedMint.sol#L41 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/ERC20/ERC20PermitPermissionedMint.sol#L53 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/ERC20/ERC20PermitPermissionedMint.sol#L65 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/ERC20/ERC20PermitPermissionedMint.sol#L76 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/ERC20/ERC20PermitPermissionedMint.sol#L94 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L159 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L166 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L177 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L184 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L191 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L199 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L53 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L61 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L69 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L82 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L93
Detailed description of the impact of this finding.
admin have privileges: admin can set address to mint any amount of frxETH, can set any address as validator, and change important state in frxETHMinter and withdraw fund from frcETHMinter
note the modifier below, either the timelock governance contract or the contract owner can access to all the high privilege function.
modifier onlyByOwnGov() { require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock"); _; }
There are numerous methods that the admin could apply to rug pull the protocol and take all user funds.
the admin can
add or remove validator from OperatorRegistry.sol set minter address or remove minter address in frxETH.sol minter set by admin can mint or burn any amount of frxETH token. set ETE deduction ratio, withdraw any amount of ETH or ERC20 token in frcETHMinter.sol
Foundry
Manual Review
Without significant redesign it is not possible to avoid the admin being able to rug pull the protocol.
As a result the recommendation is to set all admin functions behind either a timelocked DAO or at least a timelocked multisig contract.
#0 - FortisFortuna
2022-09-25T21:17:07Z
We are well aware of the permission structure. The owner will most likely be a large multisig. We mentioned the Frax Multisig in the scope too.
#1 - 0xean
2022-10-12T13:57:15Z
going to use this issue as the canonical issue for all "malicious owner" type reports. The protocol does have some serious "trust" in the administrator and the highlighted issues are important for end users to understand and should be part of the report.
π Selected for report: ladboy233
Also found by: __141345__
Detailed description of the impact of this finding.
The main risk in ETH 2.0 POS staking is the slashing penalty, in that case the frxETH will not be pegged and the validator cannot maintain a minimum 32 ETH staking balance.
https://cryptobriefing.com/ethereum-2-0-validators-slashed-staking-pool-error/
We recommand the protocol to add mechanism to ensure the frxETH is pegged via burning if case the ETH got slashed. and consider when the node do not maintain a minmum 32 ETH staking balance, who is charge of adding the ETH balance to increase the staking balance or withdraw the ETH and distribute the fund.
#0 - FortisFortuna
2022-09-26T01:41:26Z
We as the team can either choose to subsidize this, or let it float. ETH 2.0 does not allow unstaking yet. When it eventually does, we will redeploy this minting contract with updated logic that may be helpful.
#1 - 0xean
2022-10-13T23:43:16Z
I think this is valid but should be downgraded to M. Users should be aware that there is no mechanism built in to deal with slashing and that the asset backed guarantee isn't without some (perhaps negligible) risk of slashing.
https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L114 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/ERC20/ERC20PermitPermissionedMint.sol#L84
Detailed description of the impact of this finding.
In the function
function removeValidator(uint256 remove_idx, bool dont_care_about_ordering) public onlyByOwnGov {
if the flag don't care about ordering is set to False, meaning we care about about the order.
we will enter the code block below:
// Save the original validators Validator[] memory original_validators = validators; // Clear the original validators list delete validators; // Fill the new validators array with all except the value to remove for (uint256 i = 0; i < original_validators.length; ++i) { if (i != remove_idx) { validators.push(original_validators[i]); } }
the code copy the original validators, remove the validators and rebuild the validator array.
Given the unknown length of the validator array, this unbounded loop may cause removeValidator() to fail.
same issue exists in the ERC20PermitPermissionedMint.sol
// 'Delete' from the array by setting the address to 0x0 for (uint i = 0; i < minters_array.length; i++){ if (minters_array[i] == minter_address) { minters_array[i] = address(0); // This will leave a null in the array and keep the indices the same break; } }
Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
There are no upper bounds on the number of validator address looped in the code below and the the number of minter address checked in the loop below
for (uint256 i = 0; i < original_validators.length; ++i) { if (i != remove_idx) { validators.push(original_validators[i]); } }
// 'Delete' from the array by setting the address to 0x0 for (uint i = 0; i < minters_array.length; i++){ if (minters_array[i] == minter_address) { minters_array[i] = address(0); // This will leave a null in the array and keep the indices the same break; } }
Foundury
Manual Review
Have an upper bound on the number of validator address checked and have an upper bound on the number of minter address checked.
#0 - FortisFortuna
2022-09-25T22:41:39Z
Technically correct, but in practice, the number of minters will always remain low. If it becomes an issue, we can designate one minter as a "pre-minter" that has a batch of tokens minted to it beforehand, then auxiliary contracts can connect to that instead of ERC20PermitPermissionedMint.sol instead.
#1 - joestakey
2022-09-26T15:44:09Z
Duplicate of #12
π Selected for report: rotcivegaf
Also found by: 0x040, 0x1f8b, 0x4non, 0xNazgul, 0xSmartContract, 0xf15ers, 8olidity, Aymen0909, B2, Bahurum, Bnke0x0, Ch_301, CodingNameKiki, Deivitto, Diana, Funen, IllIllI, JC, JLevick, KIntern_NA, Lambda, OptimismSec, PaludoX0, RockingMiles, Rolezn, Sm4rty, Soosh, Tagir2003, Tointer, TomJ, Triangle, Trust, V_B, Waze, Yiko, __141345__, a12jmx, ajtra, asutorufos, ayeslick, aysha, bbuddha, bharg4v, bobirichman, brgltd, bytera, c3phas, cryptostellar5, cryptphi, csanuragjain, datapunk, delfin454000, durianSausage, exd0tpy, gogo, got_targ, jag, joestakey, karanctf, ladboy233, leosathya, lukris02, mics, millersplanet, natzuu, neko_nyaa, obront, oyc_109, parashar, peritoflores, rbserver, ret2basic, rokinot, ronnyx2017, rvierdiiev, sach1r0, seyni, sikorico, slowmoses, tnevler, yasir, yongskiws
28.0172 USDC - $28.02
Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.
https://swcregistry.io/docs/SWC-103
pragma solidity ^0.8.0;
pragma solidity ^0.8.0;
pragma solidity ^0.8.0;
pragma solidity ^0.8.0;
contract OperatorRegistry is Owned
after the minter is removed, it becomes address(0), which occupy storage space for minter array.
// 'Delete' from the array by setting the address to 0x0 for (uint i = 0; i < minters_array.length; i++){ if (minters_array[i] == minter_address) { minters_array[i] = address(0); // This will leave a null in the array and keep the indices the same break; } }
the 32 ETH staking balance is hardcoded
uint256 public constant DEPOSIT_SIZE = 32 ether; // ETH 2.0 minimum deposit size
If in the future, the minmum stkaing balance is changed in the etherenum network, the contract would not be functioning well.
We recommand the project make the DEPOSIT_SIZE configurable.
π Selected for report: pfapostol
Also found by: 0x040, 0x1f8b, 0x4non, 0x5rings, 0xA5DF, 0xNazgul, 0xSmartContract, 0xmatt, 0xsam, Amithuddar, Aymen0909, B2, Ben, Bnke0x0, Chom, CodingNameKiki, Deivitto, Diana, Fitraldys, Funen, IllIllI, JAGADESH, JC, Metatron, Ocean_Sky, PaludoX0, Pheonix, RaymondFam, ReyAdmirado, RockingMiles, Rohan16, Rolezn, Satyam_Sharma, Sm4rty, SnowMan, SooYa, Tagir2003, TomJ, Tomio, Triangle, V_B, Waze, __141345__, ajtra, albincsergo, asutorufos, aysha, beardofginger, bobirichman, brgltd, bulej93, bytera, c3phas, ch0bu, cryptostellar5, cryptphi, d3e4, delfin454000, dharma09, drdr, durianSausage, emrekocak, erictee, fatherOfBlocks, gogo, got_targ, imare, jag, karanctf, ladboy233, leosathya, lukris02, medikko, mics, millersplanet, natzuu, neko_nyaa, oyc_109, peanuts, prasantgupta52, rbserver, ret2basic, rokinot, ronnyx2017, rotcivegaf, sach1r0, samruna, seyni, slowmoses, tnevler, wagmi, zishansami
12.8108 USDC - $12.81
for (uint256 i = 0; i < numDeposits; ++i) {
for (uint256 i = 0; i < arrayLength; ++i) {
for (uint256 i = 0; i < times; ++i) {
for (uint256 i = 0; i < original_validators.length; ++i) {
for (uint i = 0; i < minters_array.length; i++){
function popValidators(uint256 times) public onlyByOwnGov {
function removeValidator(uint256 remove_idx, bool dont_care_about_ordering) public onlyByOwnGov {
function minter_burn_from(address b_address, uint256 b_amount) public onlyMinters {
function minter_mint(address m_address, uint256 m_amount) public onlyMinters {
function addMinter(address minter_address) public onlyByOwnGov {
function removeMinter(address minter_address) public onlyByOwnGov {
function setTimelock(address _timelock_address) public onlyByOwnGov {
The overheads outlined below are PER LOOP, excluding the first loop
storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas) Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
for (uint256 i = 0; i < original_validators.length; ++i) {
for (uint i = 0; i < minters_array.length; i++){
Saves 6 gas per loop
for (uint i = 0; i < minters_array.length; i++){