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: 5/8
Findings: 5
Award: $2,912.73
🌟 Selected for report: 3
🚀 Solo Findings: 0
pauliax
contract Withdrawable function withdraw() does not subtract from pendingWithdrawals thus it only increases and could make function getReserveBalance() revert when the balance < pendingWithdrawals.
Add this line to withdraw(): pendingWithdrawals = pendingWithdrawals.sub(reserveAmount);
#0 - fairside-core
2021-05-30T13:08:33Z
Duplicate of #72, should be increased in severity.
pauliax
function addRegistrationTributeGovernance mistakenly calls _addTribute. Function_addGovernanceTribute is never called thus governance tributes are never set nor updated.
Replace '_addTribute' with '_addGovernanceTribute' in function addRegistrationTributeGovernance.
#0 - fairside-core
2021-05-30T13:30:30Z
Duplicate of #20
pauliax
function getEtherPrice() invokes ETH_CHAINLINK.latestRoundData(). However, there are no checks if the return value indicates stale data. This could lead to stale prices according to the Chainlink documentation: “if answeredInRound < roundId could indicate stale data.” “A timestamp with zero value means the round is not complete and should not be used.”
This issue was originally described by the leading hacker @cmichelio (kudos to him) in Maple finance contest: https://github.com/code-423n4/2021-04-maple-findings/issues/82
Add missing checks for stale data. See example here: https://github.com/cryptexfinance/contracts/blob/master/contracts/oracles/ChainlinkOracle.sol#L58-L65
#0 - fairside-core
2021-05-30T13:22:02Z
Duplicate of #70
🌟 Selected for report: pauliax
480.3745 USDC - $480.37
pauliax
convictionless can be set via function setConvictionless, however, it is not used anywhere across the system, thus making it useless. Based on the comment above this variable, I expect to see it used in functions like _updateConvictionScore.
Either remove this mapping or use it where intended.
#0 - fairside-core
2021-05-30T13:51:41Z
Quite strange no one else identified this one! The absence of usage was a merging mistake, this particular mapping is slightly important to the overall operation of FairSide as certain parties should not accrue conviction, such as the Governance wallet. I believe it should be increased to medium level severity.
#1 - fairside-core
2021-06-01T13:03:59Z
Fixed in PR#10.
🌟 Selected for report: pauliax
480.3745 USDC - $480.37
pauliax
A variable named fairSideConviction is set in the contract FSD function setFairSideConviction. However, functions that use this variable do not check if it is already initialized. For example, function tokenizeConviction in contract ERC20ConvictionScore may transfer tokens to the 0x0 address: _transfer(msg.sender, address(fairSideConviction), locked); This will make these tokens inaccessible and basically burned. It would be better if the code explicitly checked before that address(fairSideConviction) != address(0). Rating this as low cuz I expect that in practice these variables will be initialized as soon as possible.
Also, this may be an additional small issue but I think it would make sense if functions setFairSideConviction and setFairSideNetwork do explicitly check that the parameter is not 0x0 address as theoretically it is possible to invoke these functions again and again when the address is empty.
require address(fairSideConviction) != address(0) where this variable is used. Same can be applied to fsdNetwork variable.
#0 - fairside-core
2021-05-30T14:01:15Z
This function is invoked directly in the deployment script and cannot be raced, as such, I think this should be set as non-critical (0).
#1 - cemozerr
2021-06-07T20:04:34Z
Labeling this as low risk, as the issue could pose a problem in case the deployment script has a bug.
🌟 Selected for report: pauliax
0 USDC - $0.00
pauliax
function _addTribute can reuse lastTribute to reduce the numbers of storage access: tributes[totalTributes - 1].amount = add224(...) can be replaced with lastTribute.amount = add224(...) as it is already a storage pointer that can be assigned a value with no need to recalculate the index and access the array again. Same situation with function _addGovernanceTribute governanceTributes.
lastTribute.amount = add224(...)
#0 - fairside-core
2021-06-01T15:07:31Z
Fixed in PR#23.