Platform: Code4rena
Start Date: 12/04/2023
Pot Size: $60,500 USDC
Total HM: 21
Participants: 199
Period: 7 days
Judge: hansfriese
Total Solo HM: 5
Id: 231
League: ETH
Rank: 19/199
Findings: 1
Award: $463.75
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: juancito
Also found by: 0xAgro, 0xNorman, 0xSmartContract, 0xStalin, 0xTheC0der, 0xWaitress, 0xhacksmithh, 0xnev, 3dgeville, 8olidity, Arz, Aymen0909, BGSecurity, BRONZEDISC, Bauchibred, Bauer, BenRai, ChainHunters, ChrisTina, CodeFoxInc, DedOhWale, DishWasher, EloiManuel, IceBear, Inspex, Jorgect, Kaysoft, LeoGold, LewisBroadhurst, Madalad, MiloTruck, MohammedRizwan, Nyx, Polaris_tow, RaymondFam, SaharDevep, SanketKogekar, Sathish9098, SolidityATL, Udsen, W0RR1O, aria, ayden, berlin-101, bin2chen, catellatech, codeslide, crc32, decade, descharre, evmboi32, eyexploit, fatherOfBlocks, georgits, giovannidisiena, joestakey, karanctf, kodyvim, ltyu, lukris02, m9800, matrix_0wl, mov, mrpathfindr, nadin, niser93, p0wd3r, parlayan_yildizlar_takimi, pavankv, pontifex, qpzm, ravikiranweb3, rbserver, santipu_, shealtielanz, slvDev, tnevler, wonjun, xmxanuel, yixxas
463.7456 USDC - $463.75
LOW
It can be assumed that the reviewers of suggested minters are invested in cryptocurrency and rather financially well off. Therefore it can be concluded that they are an audience worth attacking for a malicious actor. In context of Web3 attack vectors one should not ignore that Web2 attack vectors are still relevant.
The _message parameter of the suggestMinter() function in Frankencoin.sol (https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L83) allows the submission of additional information regarding the suggested minter. A malicious actor may for example include links that lead to phishing attacks, malware infection, ransomware attacks, scams and frauds.
Although suggesting a minter costs an attacker the minimum fee to be reviewed (if totalSupply > 0), the highly targeted audience of minter reviewers may still be worth the investment for the attacker. In an extreme case a reviewer could be pressed to act against the interests of the Frankencoin project and its Frankencoin Pool Shares holders e.g. to not veto a suggested minter which could introduce a malicious minting strategy and drain the project.
Manual review
Either don't allow links to be passed in the _message parameter of the suggestMinter() function or sanitize submitted links and inform the reviewers about the potential risks of included links BEFORE the review. It is assumed that the review of suggested minters will be enabled by the Frankencoin project through some web frontend. This would available to eligible reviewers to make the review process convenient (not requiring to interact with on-chain information and contracts directly). Here a warning could be displayed that needs to be acknowledged before the review can begin.
LOW
The _minApplicationPeriod parameter in Frankencoin.sol constructor is not sanity checked: https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L59
The missing check could lead to it being set to low or too high. It needs to be set in seconds but could accidentally be set in higher or lower units (days/milliseconds) or correctly in seconds but too low or too high. There is no proper NatSpec in place for the constructor which increases the risk that the comments on the constructor are overlooked.
Worst case of setting it too low or even to 0: This would lead to minters being active instantly (if _minApplicationPeriod is 0) or to a review period quickly passing and automatically activating the minter before any review can be done and a potential negative vote can be cast. This puts the project at risk to e.g. minting an infinite number of Frankencoins via a malicious minter.
Worst case of setting a too high value: With a very high value the time could be so long that minters would never pass the _minApplicationPeriod. This fails the idea of the project by allowing others to introduce new minters and it would be stuck with the initially introduced 2 trusted minters by the Frankencoin team. This can be considered as DoS for minter suggestion.
Manual review
Introduce a sanity check in the Frankencoin.sol constructor for _minApplicationPeriod e.g. the _minApplicationPeriod > 3 days or > 7 days. I have seen both numbers in the code so not sure which one would be the correct sanity check.
LOW
The constructor in https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/StablecoinBridge.sol#L26 does not check for the passed arguments. This could in the worst case open the door for exploits or in the most positive case require a redeploy of the StablecoinBridge contract.
Manual review
Add validation / sanity checks for all constructor arguments
Passed addresses should at least be checked or not being the zero address
The passed limit_ parameter should be checked for being in a valid range (not 0, not too low, not too high).
LOW
The index for retrieving the address to wipe is hardcoded to 0 in https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L313.
In consequence only the first address in addressesToWipe will ever be used to burn tokens in the for loop and the function does not work as expected.
The function must now be called multiple times with a single address each to wipe a larger amount of addresses. This can be cumbersome and may cause a delay that prevents a quick wipe of multiple addresses.
Manual review
Use i variable instead of hardcoded 0 when retrieving the address to wipe in the for loop.
LOW
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/ERC20.sol#L47 uses "1 << 255" to define infinity. Why the current way of defining infinity is used is unclear since the maximum value in solidity (closest to infinity) would be "type(uint256).max" which has been established as a common practice to be used.
Manual review
Use type(uint256).max to define infinity or add comment to explain why 1 << 255 is used.
NON-CRITICAL
_cubicRoot is not using _power3 in https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MathUtil.sol#L24 "_mulD18(_mulD18(x, x), x)" is used which could be implemented using "_power3(x)" instead.
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MathUtil.sol#L32 is using brackets where https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MathUtil.sol#L36 is not using brackets for a similar expression.
Manual review
NON-CRITICAL
Typos lead in the best case to irritation and in the worst case to wrong assumptions which may hurt the project.
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L78 "Complex proposals should have application periods and applications fees above the minimum." -> "A complex proposal should have an application period and an applications fee above the minimum." (Singular for clarity!)
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L188 "are only allowed to so for tokens amounts they previously" -> "are only allowed to burn amounts they previously" (please check if the suggestion is logically correct!)
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L201 "the reserver requirement" -> "the reserve requirement"
Manual review
Fix the typos
NON-CRITICAL
Several functions are named rather subtle starting with "notify" which e.g. implies some kind of message being sent which could be event emitting. But these function do significant things, e.g. assign state variables, revert under certain conditions and are used as checks in other functions.
Since they are at least in parts called from another file one needs to dive into the function in the other contract to see that they actually do more than expected. This makes it even more likely that relevant things are overlooked (by the developers).
Affected functions:
Manual review
Don't start these function names with "notify" and rename them to reflect their actual effects.
NON-CRITICAL
Generally the documentation does not follow NatSpec (https://docs.soliditylang.org/en/v0.8.19/natspec-format.html) consistently which is a flaw. E.g. it prevents tools to extract documentation properly which impacts security.
Generally function parameters don't follow a consistent approach of prefixing them with underscore (_). There is even cases where the same function has mixed usage of underscore for its parameters (see examples below).
There are cases where documentation is not properly formatted (e.g. wrong indentation) which may hint that automated formatting for files is missing in the development environment (e.g. format on save).
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/PositionFactory.sol#L14 does not use underscore for "initPeriod" which is inconsistent. All other parameters have underscore prefixes.
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/PositionFactory.sol#L36 comment is wrongly indented (2 whitespaces too much).
Manual review
NON-CRITICAL
Examples:
Manual review
Follow the recommended contract layout
NON-CRITICAL
In https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/ERC20.sol#L130 the file /doc/infiniteallowance.md is referenced. But it is not in the repository (not in the code4rena Frankencoin repository and also not in the original repository https://github.com/Frankencoin-ZCHF/FrankenCoin/search?q=nfiniteallowance.md).
In consequence a relevant part of documentation is missing for developers / auditors which impacts their work negatively through uncertainty.
Manual review
NON-CRITICAL
https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L85 "MIN_FEE && totalSupply()" (2 whitespaces between "MIN_FEE" and "&&").
Manual review
In this particular case remove the duplicate whitespace
Generally use automated code formatting consistently across all files (e.g auto-format on save)
If you use any CI tool (continuous integration) integrate a step that checks for proper code formatting after commit. It is rather recommended to not auto-format in CI since that may alter the code unexpectetly. Rather fail the CI pipeline if the code does not meet the expected formatting. Local pre-commit hooks may also be used to trigger any validations and prevent commits if code formatting is off.
#0 - 0xA5DF
2023-04-26T16:54:12Z
#4 is dupe of #941
#1 - c4-judge
2023-05-17T06:00:59Z
hansfriese marked the issue as grade-a