Platform: Code4rena
Start Date: 20/05/2021
Pot Size: $55,000 USDC
Total HM: 19
Participants: 8
Period: 7 days
Judge: cemozer
Total Solo HM: 11
Id: 11
League: ETH
Rank: 2/8
Findings: 10
Award: $14,513.79
🌟 Selected for report: 7
🚀 Solo Findings: 2
2161.6853 USDC - $2,161.69
shw
Users have to pay twice the FSD tokens when tokenizing their convictions if the locked variable is non-zero.
The first payment is made in the function tokenizeConviction of the contract ERC20ConvictionScore (line 282), where a user transfer locked amount of FSD to the contract fairSideConviction. The second payment is made afterward when the function createConvictionNFT is called (line 304). At line 50 of the contract fairSideConviction, the user transfers locked amount of FSD to the contract fairSideConviction again.
Similarly, when a user calls acquireConviction, he received the amount of locked token twice. The first is at line 123 in the contract FairSideConviction, and the second is at line 316 in the contract ERC20ConvictionScore.
Referenced code: When tokenizing convictions: ERC20ConvictionScore.sol#L282 ERC20ConvictionScore.sol#L304 FairSideConviction.sol#L50
When acquiring convictions: ERC20ConvictionScore.sol#L314 ERC20ConvictionScore.sol#L316 FairSideConviction.sol#L123
Should only charge or send FSD tokens once. Consider removing the logic at line 281-285 and 316 in the contract ERC20ConvictionScore.
#0 - fairside-core
2021-05-30T12:55:16Z
While this is 100% an issue, I believe the high severity label is not accurate because there is no fund loss. While funds are locked twice, they are also unlocked twice which means the full NFT workflow is simply overcharging rather than causing funds to be lost etc.
As such, I believe this is a 1 (minor) finding as it merely doubles the actual funds that would otherwise be locked and unlocked in the NFT process. I would also be fine with a 2 (medium), however, I believe medium is a stretch and high is definitely disputed.
#1 - fairside-core
2021-05-30T19:21:19Z
Fixed in PR#3.
#2 - cemozerr
2021-06-07T20:18:08Z
Duplicate of issue #29 and #30. Labeling this as 2 high-risk issues as they do result to loss of funds even if temporary.
1297.0112 USDC - $1,297.01
shw
The variable pendingWithdrawals in the contract Withdrawable is not decreased after the function withdraw is called, which causes the return value of function getReserveBalance less than it should be. This bug could cause incorrect results in several critical functions related to FSD token pricing, including getFSDPrice, purchaseMembership, getMaximumBenefitPerUser, mint, and burn in the FSDNetwork and FSD contracts.
Referenced code: Withdrawable.sol#L14-L19 Withdrawable.sol#L26-L28
Affected functions: FSD.sol#L85 FSD.sol#L100 FSDNetwork.sol#L136 FSDNetwork.sol#L361 FSDNetwork.sol#L369
Add pendingWithdrawals = pendingWithdrawals.sub(reserveAmount); after line 17 in the contract Withdrawable.
#0 - fairside-core
2021-05-30T13:10:24Z
One of two easter eggs!
#1 - fairside-core
2021-05-30T19:32:22Z
Fixed in PR#5.
2161.6853 USDC - $2,161.69
shw
The function _calculateDeltaOfFSD of contract ABC incorrectly converts an int256 type parameter, _reserveDelta, to uint256 by explicit conversion, which in general results in an extremely large number when the provided parameter is negative. The extremely large number could cause a SafeMath operation sub at line 43 to revert, and thus the FSD tokens cannot be burned. (_reserveDelta is negative when burning FSD tokens)
Simply calling fsd.burn after a successful fsd.mint will trigger this bug.
Referenced code: ABC.sol#L43 ABC.sol#L49 ABC.sol#L54
Use the solidity function abs to get the _reserveDelta absolute value.
#0 - fairside-core
2021-05-30T16:58:10Z
Fixed in PR#1.
🌟 Selected for report: shw
4803.7452 USDC - $4,803.75
shw
The current implementation of the arctan formula in the contract FairSideFormula is inconsistent with the referenced paper and could cause incorrect results when the input parameter is negative. The erroneous formula affects the function calculateDeltaOfFSD and the number of FSD tokens minted or burned.
The function _arctan misses two abs on the variable a. The correct implementation should be:
function _arctan(bytes16 a) private pure returns (bytes16) { return a.mul(PI_4).sub( a.mul(a.abs().sub(ONE)).mul(APPROX_A.add(APPROX_B.mul(a.abs()))) ); }
Notice that _arctan is called by arctan, and arctan is called by arcs with ONE.sub(arcInner) provided as the input parameter. Since arcInner = MULTIPLIER_INNER_ARCTAN.mul(x).div(fS3_4) can be a large number (recall that x is the capital pool), it is possible that the parameter a is negative.
Referenced code:
FairSideFormula.sol#L45-L61 FairSideFormula.sol#L77-L85 FairSideFormula.sol#L127 ABC.sol#L38
Modify the _arctan function as above.
#0 - fairside-core
2021-05-30T19:26:35Z
Fixed in PR#4.
shw
The addRegistrationTributeGovernance function in the contract FSD includes an incorrect function, _addTribute. According to its function name, the called function should be _addGovernanceTribute instead.
Referenced code: FSD.sol#L140
Should change _addTribute to _addGovernanceTribute.
#0 - fairside-core
2021-05-30T13:27:27Z
Duplicate of #20
262.6448 USDC - $262.64
shw
The getEtherPrice function in the contract FSDNetwork fetches the ETH price from a Chainlink aggregator using the latestRoundData function. However, there are no checks on roundID nor timeStamp, resulting in stale prices.
Referenced code: FSDNetwork.sol#L376-L381
Add checks on the return data with proper revert messages if the price is stale or the round is uncomplete, for example:
(uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = ETH_CHAINLINK.latestRoundData(); require(answeredInRound >= roundID, "..."); require(timeStamp != 0, "...");
#0 - fairside-core
2021-05-30T19:45:13Z
Fixed in PR#7.
#1 - cemozerr
2021-06-08T19:42:55Z
Labeling this as medium risk as stale ether price could put funds at risk.
🌟 Selected for report: shw
1441.1236 USDC - $1,441.12
shw
The variable fShareRatio in the function purchaseMembership of contract FSDNetwork is vulnerable to manipulation by flash minting and burning, which could affect several critical logics, such as the check of enough capital in the pool (line 139-142) and the staking rewards (line 179-182).
The fShareRatio is calculated (line 136) by:
(fsd.getReserveBalance() - totalOpenRequests).mul(1 ether) / fShare;
where fsd.getReserveBalance() can be significantly increased by a user minting a large amount of FSD tokens with flash loans. In that case, the increased fShareRatio could affect the function purchaseMembership results. For example, the user could purchase the membership even if the fShareRatio is < 100% previously, or the user could earn more staking rewards than before to reduce the membership fees. Although performing flash minting and burning might not be profitable overall since a 3.5% tribute fee is required when burning FSD tokens, it is still important to be aware of the possible manipulation of fShareRatio.
Referenced code: FSDNetwork.sol#L134-L142 FSDNetwork.sol#L178-L182
Force users to wait for (at least) a block to prevent flash minting and burning.
#0 - fairside-core
2021-05-30T12:46:52Z
I believe this to be a minor (1) or none (0) severity issue given that the manipulation of fShareRatio is unsustainable due to the fee and the example given is actually not possible. If I affect fShareRatio to go above 100% to purchase a membership, I will be unable to burn the necessary FSD to go below 100% again as burning is disabled when the ratio is or would go to below 100%.
#1 - fairside-core
2021-05-30T17:16:05Z
Fixed in PR#2.
#2 - cemozerr
2021-06-08T20:22:45Z
Labeling this as low risk as 3.5% tribute fee makes it very unlikely that these flash minting will be profitable.
216.1685 USDC - $216.17
shw
In most contracts, the pragma statements are declared as pragma solidity >=0.6.0 <0.8.0;, which are unlocked and could cause the contracts to accidentally be compiled or deployed using an outdated or buggy compiler version.
Referenced code:
Please use grep -R pragma . to find the unlocked pragma statements.
Should lock pragmas to a specific compiler version. Besides, consider the known compiler bugs in the following references and check whether the contracts include those bugs.
Solidity compiler bugs: Solidity repo - known bugs Solidity repo - bugs by version
#0 - fairside-core
2021-05-30T13:33:37Z
The pragma statements were left unlocked to allow flexibility in development. I believe that since this is not a functional finding, it should be marked as 0 (non-critical).
#1 - cemozerr
2021-06-07T20:09:26Z
Duplicate of https://github.com/code-423n4/2021-05-fairside-findings/issues/25. Labeling it as low risk as it could indeed cause the contracts to accidentally be compiled or deployed using an outdated or buggy compiler version
🌟 Selected for report: shw
480.3745 USDC - $480.37
shw
Users can pay fewer FSD tokens when purchasing a membership or opening a cost share request by flash minting and burning FSD tokens, which could significantly affect the FSD spot price.
The function getFSDPrice returns the current FSD price based on the reserves in the capital pool (see lines 353-364 in contract FSDNetwork). Notice that when minting and burning FSD tokens, the fsd.getReserveBalance() increases but not the fShare. Therefore, according to the pricing formula, FairSideFormula.f, the FSD price increases when minting, and vice versa, decreases when burning.
When purchasing a membership, the number of FSD tokens that a user should pay is calculated based on the current FSD price, which is vulnerable to manipulation by flash minting and burning. Consider a user performing the following actions (all are done within a single transaction or flashbot bundle):
purchaseMembership. Since the price of FSD is relatively high, the user pays fewer FSD tokens for the membership fee than before.Although the user pays for the 3.5% tribute fees, it is still possible to make a profit. Suppose that the price of FSD to ETH is p_1 and p_2 before and after minting, respectively. The user purchases a membership with x ETH costShareBenefit, and uses y ETH to flash mint the FSD tokens. In a regular purchase, the user pays 0.04x / p_1 FSD tokens, equivalent to 0.04x ETH. By performing flash mints and burns, the user pays 0.04x / p_2 FSD tokens, which is, in fact, equivalent to 0.04x * p_1 / p_2 ETH. He also pays 0.035y ETH for tribute fees. The profit user made is 0.04x * (1 - p1 / p2) - 0.035y (ETH), where p2 and y are dependent to each other but independent to x. Thus, the profit can be positive if costShareBenefit is large enough.
The same vulnerability exists when a user opens a cost share request, where the bounty to pay is calculated based on the current price of FSD tokens.
Referenced code: FSDNetwork.sol#L353-L364 FSDNetwork.sol#L145-L146 FSDNetwork.sol#L217-L222
Force users to wait for (at least) a block to prevent flash minting and burning.
#0 - fairside-core
2021-05-30T12:41:17Z
The issue relies on costShareBenefit being large enough which is inherently limited to a % of the capital pool, meaning that the arbitrage opportunity present here is inexistent or highly unlikely to be beneficial. Can we reach out to the submitter to request them to prove that even with the costShareBenefit % limit this is a sustainable attack by providing us with numbers? If no such numbers are present, I would decrease the severity of this to either minor (1) or none (0).
#1 - cemozerr
2021-06-08T20:18:32Z
Will wait for a proof from the auditer, shw, for this one.
#2 - x9453
2021-06-12T09:38:32Z
Hi, thanks for giving me a chance to clarify this finding.
After realizing that a user's costShareBenefit is limited to a % of the capital pool (5% as specified in the code), I would say this attack is not successful according to the following estimation of the upper-bound of user's profit:
User's profit = 0.04x * (1 - p1 / p2) - 0.035y <= 0.04x - 0.035y <= 0.04 * 0.05 * (z + y) - 0.035y = 0.002z - 0.033y
where z is the amount of ETH in the capital pool before minting. A negative coefficient of y implies that using a flash loan does not help to increase the profit but to decrease it.
#3 - x9453
2021-06-14T08:50:50Z
After some thoughts, I think the estimation should also consider how flash loan affects on the FSD's price to be more accurate. According to the price formula, we have p1 = A + z^4 / (C * fShare^3) and p2 = A + (z + y)^4 / (C * fShare^3) (assuming the best case, where fShare does not increase). Let r = y / z, the ratio of flash loan to the capital pool, then we can approximate p1 / p2 = z^4 / (z + y)^4 = 1 / (r + 1)^4. Therefore,
User's profit = 0.04x * (1 - p1 / p2) - 0.035y = 0.04 * 0.05 * (z + y) * (1 - 1 / (r + 1)^4) - 0.035y = (0.002 * (r + 1) * (1 - 1 / (r + 1)^4) - 0.035r) * z = f(r) * z
WolframAlpha tells us that f(r) < 0 for all r > 0, meaning that the user does not make a profit no matter how much flash loan he borrowed.
It is worth mentioning that different % of withdrawal fee, cost share benefit limit, and tribute fee could lead to different results. That is, the constants, 0.002 and 0.035, determine whether user's profit can be positive (i.e., there exists r > 0 s.t. f(r) > 0). Further calculation shows that this happens iff the product of the withdrawal fee and cost share benefit limit is greater than the tribute fee divided by 4, which is unlikely in normal settings. Please let me know if you need more details or a PoC on this.
🌟 Selected for report: shw
0 USDC - $0.00
shw
Gas optimization is possible for the current rootPows implementation.
The original implementation of rootPows requires 4 mul and 2 sqrt:
function rootPows(bytes16 x) private pure returns (bytes16, bytes16) { // fourth root x = x.sqrt().sqrt(); // to the power of 3 x = _pow3(x); // we offset the root on the second arg return (x, x.mul(x)); }
However, the calculation process can be simplified to be more gas-efficient than the original with only 1 mul and 2 sqrt requried:
function rootPows(bytes16 x) private pure returns (bytes16, bytes16) { bytes16 x1_2 = x.sqrt(); bytes16 x3_2 = x.mul(x1_2); bytes16 x3_4 = x3_2.sqrt(); return (x3_4, x3_2); }
Referenced code: FairSideFormula.sol#L67-L75
To save gas, change the implementation of rootPows as mentioned above.
#0 - fairside-core
2021-05-30T13:13:14Z
Optimization is confirmed (basically constructs x^3/2 then applies root on it). Given that this is a gas optimization, perhaps the severity should be noted down to 1? I'll leave this up to the judges.
#1 - fairside-core
2021-05-30T19:36:55Z
Fixed in PR#6.
#2 - cemozerr
2021-06-07T20:15:26Z
Labeling this as gas optimization.