Platform: Code4rena
Start Date: 19/10/2021
Pot Size: $30,000 ETH
Total HM: 5
Participants: 13
Period: 3 days
Judge: GalloDaSballo
Total Solo HM: 4
Id: 43
League: ETH
Rank: 5/13
Findings: 2
Award: $2,778.21
🌟 Selected for report: 8
🚀 Solo Findings: 0
🌟 Selected for report: pants
0.2998 ETH - $1,072.34
pants
The following calculation can be more numeric precise: uint128 perEpochRateIncrease =uint128(uint256(allocatedTokensPerEpoch)*divider/uint256(totalGlobalShares));
globalExchangeRate += perEpochRateIncrease * (currentEpoch - lastUpdateEpoch);
Change it to: uint128 perEpochRateIncrease =uint256(allocatedTokensPerEpoch)*divider; globalExchangeRate += perEpochRateIncrease * (currentEpoch - lastUpdateEpoch) / uint256(totalGlobalShares);
#0 - kitti-katy
2021-10-21T19:17:18Z
uint128 perEpochRateIncrease =uint256(allocatedTokensPerEpoch)*divider; would break.
#1 - GalloDaSballo
2021-11-02T00:57:39Z
The sponsor has applied one of the improvements in the finding
pants
CQT.transfer / CQT.transferFrom should be CQT.safeTransfer / CQT.safeTransferFrom for the case you choose to use USDT as the CQT for example.
#0 - kitti-katy
2021-10-21T19:13:53Z
duplicate of #1
#1 - GalloDaSballo
2021-11-02T01:09:10Z
Marking to gas as per the duplicate
pants
Some functions including recoverUnstaking, redeemAllRewards, etc could be set external instead of public. There is no use of those inside the cotract.
#0 - kitti-katy
2021-10-21T19:23:40Z
duplicate of #2
#1 - GalloDaSballo
2021-11-01T16:41:55Z
Duplicate of #2
🌟 Selected for report: pants
0.0304 ETH - $108.84
pants
You can change lines 377, 378
from uint128 addEpochs = futureRewards/amount; require(addEpochs != 0, "This amount will end the program");
to require(futureRewards!= 0, "This amount will end the program"); uint128 addEpochs = futureRewards/amount;
Would save some gas and is more elegant.
#0 - kitti-katy
2021-10-21T21:15:24Z
Not quite.
futureRewards might be less than amount, this will result in addEpochs being 0.
So the check should actually be:
require(futureRewards >= amount, "This amount will end the program"); uint128 addEpochs = futureRewards/amount;
#1 - GalloDaSballo
2021-11-01T16:41:28Z
Agree with the warden in using the require early (fail fast) Also agree with refactoring from the sponsor
Will mark finding as valid, but believe the sponsor can add the improvement as they see fit
🌟 Selected for report: pants
0.0304 ETH - $108.84
pants
Change lines 178 to 173. We know it's minor but it's still optimizing gas and more elegant :)
#0 - GalloDaSballo
2021-11-01T16:39:42Z
The sponsor has applied the improvement
🌟 Selected for report: pants
0.0304 ETH - $108.84
pants
Declaration inside a loop is less gas efficient than before it. See line 462 for example.
#0 - GalloDaSballo
2021-11-01T16:43:29Z
The sponsor confirms
From the attached commit (which only shows a .json file) the improvement was not applied
🌟 Selected for report: WatchPug
Also found by: harleythedog, pants
0.0082 ETH - $29.39
pants
the storage state variable validatorsN is called inside a loop instead of caching. For example at line 440. We would first cache it and only then use it to save gas.
#0 - kitti-katy
2021-10-26T18:44:22Z
#1 - GalloDaSballo
2021-11-01T16:44:17Z
Duplicate of #53
🌟 Selected for report: pants
0.0304 ETH - $108.84
pants
++i is more gas efficient than i++ in loops forwarding. At line 440 for example. We would also recommend to use unchecked{++i} and change i declaration to uint256
#0 - kitti-katy
2021-10-21T19:18:54Z
The get functions were designed not to be used in actual transactions. But yes, can switch to ++i.
#1 - GalloDaSballo
2021-11-01T16:50:12Z
Preincrement is cheaper (about 5 gas per iteration)
0.0082 ETH - $29.39
pants
state variable divider could be set immutable. At line 9.
#0 - GalloDaSballo
2021-11-01T16:54:10Z
Agree with finding, constant saves gas
🌟 Selected for report: pants
0.0304 ETH - $108.84
pants
At DelegatedStaking.sol, line 11
delegatorCoolDown could be set immutable.
Furthermore, you could set it constant with the value 28*6646.
#0 - kitti-katy
2021-10-21T19:13:08Z
might change it in the future upgrade.
(Had to state it in the requirements)
#1 - GalloDaSballo
2021-11-01T16:52:41Z
Agree with the warden, the variable is never changed Since it's set in initialize the most logical way to save gas would be convert to a constant (as immutable doesn't work with upgradeable contracts)
Will set the finding as valid because of it's utility, and understand this can be a nofix for code
🌟 Selected for report: pants
0.2998 ETH - $1,072.34
pants
At line 345, the function addValidatior doesn't check new validator address != 0.
#0 - kitti-katy
2021-10-21T17:48:38Z
That is true but it is called only by the owner, so we assume this will never happen or if it happens it will not hurt anything, literally as we can disable that validator instance right away.
#1 - GalloDaSballo
2021-11-02T01:01:41Z
I understand the sponsor's point of view, however, I'll make the case for the warden finding for 2 reasons:
Will leave the finding at Severity Low as per the documentation:
State handling, function incorrect as to spec, issues with comments.
In this case setting a validator with address 0 would be incorrect as to spec