Platform: Code4rena
Start Date: 09/11/2021
Pot Size: $30,000 ETH
Total HM: 6
Participants: 17
Period: 3 days
Judge: pauliax
Total Solo HM: 3
Id: 50
League: ETH
Rank: 10/17
Findings: 2
Award: $314.43
🌟 Selected for report: 3
🚀 Solo Findings: 0
0.0239 ETH - $112.98
ye0lde
If integer underflow occurred in variable membership[beneficiary].availableCostShareBenefits it appears it would have immediate consequences as a "payclaim" call is done on the next line. This would set the "beneficiary" up to withdraw a large amount exceeding his actual CSR amount.
Other variables in this function could also underflow and cause problems..
DSMath functions are not used to protect against integer underflow in _processCostShareRequest. While this function is "internal" and only callable by governance or timelock contracts there are no checks for consistency in the caller updateCostShareRequest. openCostShareRequest where the CSR is originally created is where the consistency check occurs.
There is a long path between the initial consistency checks and _processCostShareRequest.
I suggest that there is too much complexity and consequence to rely on just the initial consistency check to protect against underflow.
Possible underflows are here: https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/network/FSDNetwork.sol#L572 https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/network/FSDNetwork.sol#L576-L577 https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/network/FSDNetwork.sol#L588
VS Code, Remix
Use DSMath (.sub) to protect against underflows.
#0 - YunChe404
2021-11-14T16:38:25Z
_processCostShareRequest inputs are data directly fetched from the costShareRequests mapping, hence cannot the operations can never underflow.
#1 - pauliax
2021-11-19T11:02:08Z
A duplicate of #71, please read the comment there.
🌟 Selected for report: ye0lde
Also found by: WatchPug, elprofesor, pants
0.0028 ETH - $13.41
ye0lde
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
I can see that the sponsor is committed to verbose revert strings as almost every revert string is > 32 bytes but I wanted to at least mention this issue.
Almost every revert string in the project is > 32 bytes.
Visual Studio Code
Consider shortening the revert strings to fit in 32 bytes or using custom errors (v0.8.4) in the future.
#0 - YunChe404
2021-11-14T11:18:47Z
#15
#1 - pauliax
2021-11-16T21:49:54Z
Valid optimization. Making this a primary issue as it contains a clear explanation and suggestions.
ye0lde
Removing unused named return variables can reduce gas usage and improve code clarity.
Visual Studio Code, Remix
Remove the unused named returns
#0 - YunChe404
2021-11-14T11:17:54Z
The first bullet is a duplicate of #21
#1 - pauliax
2021-11-16T23:02:17Z
A duplicate of #21 yet this issue mentions 2 places where the suggestion can be applied.
🌟 Selected for report: ye0lde
0.0155 ETH - $73.58
ye0lde
Using existing local variables instead of reading state variables will save gas by converting SLOADs to MLOADs.
delay_ can be used here: https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/timelock/Timelock.sol#L145
pendingAdmin_ can be used here: https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/timelock/Timelock.sol#L181
Visual Studio Code, Remix
See Proof of Concept
#0 - pauliax
2021-11-16T23:23:27Z
Valid optimization.
0.0172 ETH - $81.35
ye0lde
Open TODOs can point to architecture or programming issues that still need to be resolved.
The TODOs are here: https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/token/ABC.sol#L53 https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/network/FSDNetwork.sol#L521
VS Code
Consider resolving the TODOs before deploying.
#0 - YunChe404
2021-11-14T11:18:21Z
#14
#1 - pauliax
2021-11-17T15:01:41Z
Valid suggestion. Making this a primary issue as it contains links to the concrete LOC.
Also grouping this issue together with others about TODO / misleading comment issues. Based on previous experience such issues deserve a severity of low: https://github.com/code-423n4/2021-10-tempus-findings/issues/39 or https://github.com/code-423n4/2021-09-swivel-findings/issues/67
#2 - YunChe404
2021-11-21T10:34:13Z
The exhibit does not pose an actual security vulnerability, hence the severity should be lowered to level zero.
#3 - pauliax
2021-11-22T07:33:43Z
As I mentioned this is based on experience from previous contests. Also, from official guidelines: "Low: Assets are not at risk. State handling, function incorrect as to spec, issues with comments."