Platform: Code4rena
Start Date: 04/01/2022
Pot Size: $25,000 USDC
Total HM: 3
Participants: 40
Period: 3 days
Judge: Ivo Georgiev
Total Solo HM: 1
Id: 75
League: ETH
Rank: 16/40
Findings: 2
Award: $258.48
🌟 Selected for report: 4
🚀 Solo Findings: 0
🌟 Selected for report: TomFrenchBlockchain
142.4013 USDC - $142.40
TomFrenchBlockchain
Reduced readability
In a number of placed we seem to be inlining an assignment with the usage of that variable:
This is quite atypical in my experience and reduces readability: lines which contain require statements and event emission now modify contract storage.
Consider whether any small benefits to gas/compactness are worth the reduced clarity.
#0 - deluca-mike
2022-01-05T09:30:22Z
Agreed. Will go with the option to assign return variables in order to keep the function prototype pattern/style consistent (i.e. return variables all have names).
#1 - deluca-mike
2022-01-13T21:07:52Z
You can see in the release candidate contract that these are all fixed, but specifically:
emergencyUnlock (did not exist before)locklockWithPermitwithdrawableOfgetScore (formerly getPoints)scoreOf (formerly pointsOf)tokenURI_generateNewTokenId_getScore (formerly _getPoints)_getScoreFromTokenId (formerly _getPointsFromTokenId)_lock (former _lock became _createLockedPosition, so this function is new)_updateDistributableXDEFI (formerly _updateXDEFIBalance)_withdrawableGiven7.6096 USDC - $7.61
TomFrenchBlockchain
Reduction of potential gas refunds.
The reentrancy guard variable is initially set to zero, set to a nonzero value and then reset to zero:
We then have to the higher cost for writing to clean storage rather than dirty storage (which is then refunded). This is not recommended as it can cause the size of the gas refunded to users to be capped. For more info see the OZ implementation:
Change from 0->1->0 to 1->2->1
#0 - deluca-mike
2022-01-05T09:31:11Z
For some cases 0 -> 1 -> 0 is cheaper, but after some testing, its overall cheaper to use 1 -> 2 -> 1 (specifically in the batch functions).
#1 - deluca-mike
2022-01-13T21:00:22Z
See changes from PR and release candidate result.
#2 - Ivshti
2022-01-16T06:20:16Z
it's resolved now, valid finding
🌟 Selected for report: Dravee
Also found by: Czar102, TomFrenchBlockchain, defsec
18.2673 USDC - $18.27
TomFrenchBlockchain
Gas costs
The contracts allow users to merge, relock and unlock batches of nfts by passing an array of token IDs.
As these functions do not modify these arrays they can be changed to store the tokenIds array in calldata rather than copying it all into memory for no reason. This will save gas (especially when passing long arrays)
replace memory with calldata in function signature.
#0 - deluca-mike
2022-01-05T19:26:43Z
Yup, good catch. Will do.
#1 - deluca-mike
2022-01-09T10:38:31Z
Duplicate #29
🌟 Selected for report: TomFrenchBlockchain
Also found by: onewayfunction
45.1044 USDC - $45.10
TomFrenchBlockchain
Extra gas costs of all locking operations.
XDEFIDistribution.sol stores the total supply of the XDEFI token:
This is so that the amount being locked can be checked to be less than this on each call
This is unnecessary as the XDEFI token has no external mint function and so has a fixed supply. It's then impossible for any user to supply more than 240M XDEFI in order to fail this check.
https://etherscan.io/address/0x72b886d09c117654ab7da13a14d603001de0b777#code
Remove the unnecessary check on the total supply.
#0 - deluca-mike
2022-01-05T09:29:18Z
Good catch on MAX_TOTAL_XDEFI_SUPPLY, it should be constant and is being removed because you are correct that it is useless given that XDEFI has a fixed supply.
#1 - deluca-mike
2022-01-13T21:22:51Z
You can see in the release candidate contract, that MAX_TOTAL_XDEFI_SUPPLY has been removed, and amount_ is not longer checked to be greater than zero or less than MAX_TOTAL_XDEFI_SUPPLY, but rather that resulting units are sufficient: https://github.com/XDeFi-tech/xdefi-distribution/blob/v1.0.0-rc.0/contracts/XDEFIDistribution.sol#L333
#2 - Ivshti
2022-01-16T06:16:25Z
valid finding!
🌟 Selected for report: TomFrenchBlockchain
Also found by: WatchPug
45.1044 USDC - $45.10
TomFrenchBlockchain
Extra gas costs from unnecessary casting.
pointsCorrection is stored as a int256 variable.
However we can see that this variable is always negative (_pointsPerUnit and units are both positive)
The only usage of pointsCorrection is in the _withdrawableGiven function as shown below.
( _toUint256Safe( _toInt256Safe(_pointsPerUnit * uint256(units_)) + pointsCorrection_ ) / _pointsMultiplier ) + uint256(depositedXDEFI_);
pointsCorrection is set to _pointsPerUnit * uint256(units_) when locking and _pointsPerUnit only increases so we can safely store `pointsCorrection as a positive uint256 (note this is an assumption of the original code as well) and simplify the above expression.
// notice the sign change before `pointsCorrection_` (_pointsPerUnit * uint256(units_) - pointsCorrection_) / _pointsMultiplier + uint256(depositedXDEFI_);
We can then remove a significant amount of casting along with the associated costs.
store pointsCorrection in a uint256 and subtract rather than add.
#0 - deluca-mike
2022-01-06T08:14:26Z
This is true. Will fix. There are other implications of this elsewhere, but it's valid nonetheless.
#1 - deluca-mike
2022-01-14T04:42:38Z
Given the amount of refactoring due to various suggestions (unchecked math, gas savings, duplicate code isolation, distribution math edge cases, etc) it is difficult to isolate the changes made to satisfy this issue.
Nevertheless, it can be seen that the release candidate contract uses only unsigned math using uint256 types.
Namely, pointsCorrection is now a uint256, and the _withdrawableGiven math is all unsigned.