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: 12/40
Findings: 2
Award: $485.01
🌟 Selected for report: 4
🚀 Solo Findings: 0
210.9649 USDC - $210.96
robee
From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix. With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam. You have reachable asserts in the following locations (which should be replaced by require / are mistakenly left from development phase):
XDEFIDistribution.sol : reachable assert in line 284 XDEFIDistribution.sol : reachable assert in line 288
#0 - deluca-mike
2022-01-05T09:14:36Z
Good catch. Will be converting these to if-reverts with new custom error messages.
#1 - deluca-mike
2022-01-13T21:33:51Z
_toInt256Safe and _toUint256Safe were removed from the release candidate contract.
13.1525 USDC - $13.15
robee
The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.
The function withdrawableOf in XDEFIDistribution.sol could be set external The function tokenURI in XDEFIDistribution.sol could be set external The function getAllTokensForAccount in XDEFIDistributionHelper.sol could be set external The function getAllLockedPositionsForAccount in XDEFIDistributionHelper.sol could be set external
#0 - deluca-mike
2022-01-05T09:20:27Z
Good catches.
Except, tokenURI is public in the ERC721 contract, and thus it can not be overridden and made external, even if I wanted to.
18.2673 USDC - $18.27
robee
Prefix increments are cheaper than postfix increments.
Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change
There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)).
But increments perform overflow checks that are not necessary in this case.
These functions use not using prefix increments (++x) or not using the unchecked keyword:
just change to unchecked: XDEFIDistribution.sol, i, 80 just change to unchecked: XDEFIDistribution.sol, i, 212 just change to unchecked: XDEFIDistribution.sol, i, 325 just change to unchecked: XDEFIDistributionHelper.sol, i, 15 just change to unchecked: XDEFIDistributionHelper.sol, i, 29 just change to unchecked: XDEFIDistributionHelper.sol, i, 42
#0 - deluca-mike
2022-01-05T09:35:33Z
Prefix increments were used. In any case, we will unchecked everywhere possible, including these forr-loop locations.
#1 - deluca-mike
2022-01-13T21:31:07Z
🌟 Selected for report: robee
100.2321 USDC - $100.23
robee
There are places in the code (especially in for-each loops) that loads the same array element more than once. In such cases, only one array boundaries check should take place, and the rest are unnecessary. Therefore, this array element should be cached in a local variable and then be loaded again using this local variable, skipping the redundent second array boundaries check:
XDEFIDistributionHelper.sol, variable name: tokenIds times: 3 at: getAllLockedPositionsForAccount
#0 - deluca-mike
2022-01-05T09:47:44Z
True, but the issue is that caching tokenIds[i] there results in a STACK_TOO_DEEP error (even with full stack optimizer settings) and trying to change the IXDEFIDistribution interface so that positionOf returns a Position memory fails due to this: https://github.com/ethereum/solidity/issues/11826
Since this is just a helper contract, intended only to be used by clients (and thus no gas used, since there are no mined transactions) there is no real issue or need to fix this.
#1 - deluca-mike
2022-01-09T10:18:44Z
After going over all the issues, changes to the contract to satisfy other recommendations/issues has allowed me to no longer get STACK_TOO_DEEP with the new contracts, so I was able to implement this gas optimization.
#2 - deluca-mike
2022-01-13T21:28:04Z
Specific to this issue, getAllLockedPositionsForAccount was fixed.
For completeness, this array element caching is done in setLockPeriods twice, in merge, and in getAllTokensForAccount.
🌟 Selected for report: TomFrenchBlockchain
142.4013 USDC - $142.40
robee
Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.
XDEFIDistribution.sol, lock XDEFIDistribution.sol, lockWithPermit XDEFIDistribution.sol, withdrawableOf XDEFIDistribution.sol, getPoints XDEFIDistribution.sol, pointsOf XDEFIDistribution.sol, tokenURI XDEFIDistribution.sol, _generateNewTokenId XDEFIDistribution.sol, _getPoints XDEFIDistribution.sol, _getPointsFromTokenId XDEFIDistribution.sol, _toUint256Safe XDEFIDistribution.sol, _updateXDEFIBalance
#0 - deluca-mike
2022-01-05T09:16:15Z
Yup, we'll go with the option to assign return variables in order to keep the function prototype pattern/style consistent. However, this should be informational (i.e. no risk).
#1 - deluca-mike
2022-01-09T10:20:45Z
Duplicate #2