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: 3/8
Findings: 8
Award: $12,089.47
🌟 Selected for report: 10
🚀 Solo Findings: 2
🌟 Selected for report: 0xRajeev
4803.7452 USDC - $4,803.75
0xRajeev
Conviction scores for new addresses/users fail to initialize+bootstrap in ERC20ConvictionScore’s _updateConvictionScore() because a new user’s numCheckpoints will be zero and never gets initialized.
This effectively means that FairSide conviction scoring fails to bootstrap at all, leading to failure of the protocol’s pivotal feature.
When Alice transfers FSD tokens to Bob for the first time, _beforeTokenTransfer(Alice, Bob, 100) is triggered which calls _updateConvictionScore(Bob, 100) on Line55 of ERC20ConvictionScore.sol.
In function _updateConvictionScore(), given that this is the first time Bob is receiving FSD tokens, numCheckpoints[Bob] will be 0 (Line116) which will make ts = 0 (Line120), and Bob’s FSD balance will also be zero (Bob never has got FSD tokens prior to this) which makes convictionDelta = 0 (Line122) and not let control go past Line129.
This means that a new checkpoint never gets written, i.e. conviction score never gets initialized, for Bob or for any user for that matter.
Manual Analysis
FairSide’s adjustment of Compound’s conviction scoring is based on time and so needs an initialization to take place vs. Compound’s implementation. A new checkpoint therefore needs to be created+initialized for a new user during token transfer.
#0 - fairside-core
2021-06-01T14:51:02Z
Fixed in PR#18.
0xRajeev
During tokenization of conviction scores, the user can optionally provide FSDs to be locked to let it continue conviction accrual. However, the amount of FSDs specified for locking are debited twice from the user leading to fund loss for user.
This, in effect, forces the user to unknowingly and unintentionally lock twice the amount of FSD tokens, leading to a loss of the specified ‘locked’ number of tokens.
Alice decides to tokenise her conviction score into an NFT and specifies 100 FSD tokens to be locked in her call to tokenizeConviction(100). 100 FSD tokens are transferred from her FSD balance to FairSideConviction contract on Line282 of ERC20ConvictionScore.sol. However, in FairSideConviction.createConvictionNFT(), the specified locked amount is transferred again from Alice to the contract on Line50 of FairSideConviction.sol.
The impact is that Alice wanted to lock only 100 FSD token but the FairSide protocol has debited 200 tokens from her balance leading to a loss of 100 FSD tokens.
Manual Analysis
Remove the redundant transfer of FSD tokens on Line282 in tokenizeConviction() of ERC20ConvictionScore.sol.
#0 - fairside-core
2021-05-30T12:52:39Z
Duplicate of #74
2161.6853 USDC - $2,161.69
0xRajeev
The tokens optionally locked during tokenization are released twice on acquiring conviction back from a NFT. (The incorrect double debit of locked funds during tokenization has been filed as a separate finding because it is not necessarily related and also occurs in a different part of the code.)
When a user wants to acquire back the conviction score captured by a NFT, the FSD tokens locked, if any, are released to the user as well. However, this is incorrectly done twice. Released amount is transferred once on Line123 in _release() (via acquireConviction -> burn) of FairSideConviction.sol and again immediately after the burn on Line316 in acquireConviction() of ERC20ConvictionScore.sol.
This leads to loss of protocol funds.
Alice tokenizes her conviction score into a NFT and locks 100 FSDs. Bob buys the NFT from Alice and acquires the conviction score back from the NFT. But instead of 100 FSDs that were supposed to be locked with the NFT, Bob receives 100+100 = 200 FSDs from FairSide protocol.
Manual Analysis
Remove the redundant transfer of FSD tokens from protocol to user on Line316 in acquireConviction() of ERC20ConvictionScore.sol.
#0 - fairside-core
2021-05-30T12:52:25Z
This is directly related to #29 as it refers to the same workflow, as seen in #74 as a single submission. I believe splitting this into two findings is unfair for the first party and secondly, it does not make sense because there is a valid argument for disagreeing with the severity seen on #74. Can we close this and merge it with #29?
#1 - fairside-core
2021-06-01T14:28:20Z
Fixed in PR#3.
#2 - cemozerr
2021-06-07T19:29:38Z
Labeling issues #29 and #30 as separate issues because they both pose major issues, which lead to temporary loss of funds, in two different workflows. One when tokenizing convictions and another when acquiring convictions.
262.6448 USDC - $262.64
0xRajeev
The addRegistrationTributeGovernance() function is called by the FSD network to update tribute when 7.5% is contributed towards governance as part of purchaseMembership(). However, this function incorrectly calls _addTribute() (as done in addRegistrationTribute) instead of _addGovernanceTribute().
The impact is that governanceTributes never gets updated and the entire tribute accounting logic is rendered incorrect.
Manual Analysis
Use _addGovernanceTribute() instead of _addTribute on L140 of FSD.sol
#0 - fairside-core
2021-05-30T13:27:58Z
The second of the two easter eggs!
#1 - fairside-core
2021-06-01T14:58:38Z
Fixed in PR#20.
648.5056 USDC - $648.51
0xRajeev
liquidateDai() calls Uniswap’s swapExactTokensForETH to swap Dai to ETH. This will work if msg.sender, i.e. FSD contract, has already given the router an allowance of at least amount on the input token Dai.
Given that there is no prior approval, the call to UniswapV2 router for swapping will fail because msg.sender has not approved UniswapV2 with an allowance for the tokens being attempted to swap.
The impact is that updateCostShareRequest() will fail and revert while working with stablecoin Dai.
https://uniswap.org/docs/v2/smart-contracts/router02/#swapexacttokensfortokens
Manual Analysis
Add FSD approval to UniswapV2 with an allowance for the tokens being attempted to swap.
#0 - fairside-core
2021-06-01T14:55:46Z
Fixed in PR#19.
648.5056 USDC - $648.51
0xRajeev
_updateConvictionScore() function returns convictionDelta and governanceDelta which need to be used immediately in a call to _updateConvictionTotals(convictionDelta, governanceDelta) for updating the conviction totals of conviction and governance-enabled conviction for the entire FairSide network.
This updation of totals after a call to _updateConvictionScore() is done on Line70 in _beforeTokenTransfer() and Line367 in updateConvictionScore() of ERC20ConvictionScore.sol.
However, the return values of _updateConvictionScore() are ignored on Line284 in tokenizeConviction() and not used to update the totals using _updateConvictionTotals(convictionDelta, governanceDelta).
The impact is that when a user tokenizes their conviction score, their conviction deltas are updated and recorded (only if the funds locked are zero which is incorrect and reported separately in a different finding) but the totals are not updated. This leads to incorrect accounting of TOTAL_CONVICTION_SCORE and TOTAL_GOVERNANCE_SCORE which are used in the calculation of tributes and therefore will lead to incorrect tribute calculations.
Alice calls tokenizeConviction() to convert her conviction score into an NFT. Her conviction deltas as returned by _updateConvictionScore() are ignored and TOTAL_CONVICTION_SCORE and TOTAL_GOVERNANCE_SCORE values are not updated. As a result, the tributes rewarded are proportionally more than what should have been the case because the conviction score totals are used as the denominator in availableTribute() and availableGovernanceTribute().
Manual Analysis
Use the return values of _updateConvictionScore() function (i.e. convictionDelta and governanceDelta) on Line284 of ERC20ConvictionScore.sol and use them in a call to _updateConvictionTotals(convictionDelta, governanceDelta).
#0 - fairside-core
2021-06-01T14:45:16Z
Fixed in PR#17.
🌟 Selected for report: 0xRajeev
1441.1236 USDC - $1,441.12
0xRajeev
Besides the conviction scores of users, there appears to be tracking of the FairSide protocol’s tokenized conviction score as a whole (using fscAddress = address(fairSideConviction)). This is evident in the attempted reduction of the protocol’s score when a user acquires conviction back from a NFT. However, the complementary accrual of user's conviction score to fscAddress when user tokenizes their conviction score to mint a NFT is missing in tokenizeConviction().
Because of this missing updation of conviction score to fscAddress on tokenization, there are no checkpoints written for fscAddress and there also doesn’t appear to be any initialization for bootstrapping this address’s conviction score checkpoints. As a result, the sub224() on Line350 of ERC20ConvictionScore.sol will always fail with an underflow because fscOld = 0 (because fscNum = 0) and convictionScore > 0, effectively reverting all calls to acquireConviction().
The impact is that all tokenized NFTs can never be redeemed back to their conviction scores and therefore leads to lock/loss of FSD funds for users who tokenized/sold/bought FairSide NFTs.
Alice tokenizes her conviction score into a NFT. She sells that NFT to Bob who pays an amount commensurate with the conviction score captured by that NFT (as valued by the market) and any FSDs locked with the NFT.
Bob then attempts to redeem the bought NFT back to the conviction score to use it on FairSide network. But the call to acquireConviction() fails. Bob is never able to redeem Alice’s NFT and has lost the funds used to buy it.
Manual Analysis
Add appropriate logic to bootstrap+initialize fscAddress’s tokenized conviction score checkpoints and update it during tokenization.
#0 - fairside-core
2021-05-30T14:56:23Z
Although the finding is correct, FSDs will not be permanently locked in the NFT as they can still be redeemed via the dedicated release function on the conviction NFT implementation. As such, I would label this a medium level finding given that the conviction scores will indeed be lost.
#1 - fairside-core
2021-06-01T14:42:03Z
Fixed in PR#16.
#2 - cemozerr
2021-06-07T22:33:37Z
Labeling this as medium risk as FSDs will not be permanently locked.
0xRajeev
Zero-address checks as input validation on address parameters is always a best practice. This is especially true for critical addresses that are immutable and set in the constructor because they cannot be changed later. Accidentally using zero addresses here will lead to failing logic or force contract redeployment and increased gas costs.
Manual Analysis
Add zero-address input validation for these addresses in the constructor.
#0 - fairside-core
2021-05-30T13:59:14Z
Duplicate of #56
0xRajeev
All contracts use a Solidity compiler pragma range >=0.6.0 <0.8.0 which spans a breaking change version 0.7.0. This compiler range is very broad and includes many syntactic/semantic changes across the versions. Specifically, see silent changes in https://docs.soliditylang.org/en/v0.7.0/070-breaking-changes.html#silent-changes-of-the-semantics.
This compiler range, for example, allows testing with Solidity compiler version 0.6.x but deployment with 0.7.x. While any breaking syntactic changes will be caught at compile time, there is a risk that the silent change in 0.7.0 which applies to exponentiation/shift operand types might affect the FairSide formula or other mathematical calculations, thus breaking assumptions and accounting.
The opposite scenario may also happen where testing is performed with Solidity compiler version 0.7.x but deployment with 0.6.x, which may allow bugs fixed in 0.7.x to be present in the deployed code.
https://docs.soliditylang.org/en/v0.7.0/070-breaking-changes.html#silent-changes-of-the-semantics
https://docs.soliditylang.org/en/v0.7.0/070-breaking-changes.html#
https://docs.soliditylang.org/en/v0.8.4/bugs.html
Manual Analysis
Use the same compiler version both for testing and deployment by enforcing this in the pragma itself. An unlocked/floating pragma is risky especially one that ranges across a breaking compiler minor version.
#0 - fairside-core
2021-05-30T13:34:36Z
Duplicate of #66
🌟 Selected for report: 0xRajeev
480.3745 USDC - $480.37
0xRajeev
FairSide contracts use DappHub’s DSMath safe arithmetic library that provides overflow/underflow protection but the safe DSMath functions are not used in many places, especially in the FSD mint/burn functions.
While there do not appear to be any obvious integer overflows/underflows in the conditions envisioned, there could be exceptional paths where overflows/underflows may be triggered leading to minting/burning an unexpected number of tokens.
Manual Analysis
Use DSMath add/sub functions instead of +/- in all places.
#0 - fairside-core
2021-05-30T15:20:28Z
All linked segments are guaranteed not to overflow / underflow. In detail:
getReserveBalance always takes into account the actual balance of the contract which will always be greater-than-or-equal to msg.value.bonded amount is always a percentage of msg.valuetribute amount is always a percentage of capitalDesiredreserveWithdrawn will always be less than or equal to etherBalanceAtBurnDue to the above, I would label the finding as non-critical. In general, SafeMath utilization is avoided in any case that it can be to reduce gas costs.
#1 - cemozerr
2021-06-07T20:52:44Z
Labeling this as low risk as not using dsmath might lead to exceptional paths where overflows/underflows may be triggered, even if those paths are not enumerated above.