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: 8/40
Findings: 2
Award: $812.02
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: Czar102
781.3514 USDC - $781.35
Czar102
An owner of the contract may, by ordering their or others' locking transactions, significantly increase or decrease bonusMultiplier for some set of transactions.
So, by mining a single block, an owner can lower other's bonus multipliers, execute locking transactions and then restore bonus multipliers.
A user might send a locking transaction in a similar time as an owner lowers the multipliers, resulting in lowering the revenue against data presented to the user.
An owner can also pass ownership to a contract that will change bonus multipliers and lock funds with a very high bonus multiplier, then restore previous multipliers' state not to let others do the same. This way, the owner can gain an unfair advantage over others.
Use a timelock for setLockPeriods(...) function and require passing bonusMultiplier in locking functions, revert if they are different from the state variables.
#0 - deluca-mike
2022-01-07T00:10:57Z
It is true that the owner of the contract can set, reset, and delete lock periods and bonuses, but keep in mind that the owner is the XDEFI organization, who is also airdropping the rewards, so there is some trust needed, which is why I categorize this as low risk, since it makes the assumption that the XDEFI organization that is giving out rewards wants to act poorly and has the ability to order transactions in a block.
I agree with the need for the bonusMultiplier as a argument for users, so that it can be confirmed at the moment their transaction gets mined.
I do not agree with a timelock for changing bonus multipliers, since that requires a lot more state and calls, and just makes things more complicated when the attack assumptions are already unlikely.
#1 - deluca-mike
2022-01-14T04:30:27Z
In the release candidate contract, all locking functions take the bonusMultiplier as a argument, and check that applied bonusMultiplier is greater or equal to what the user expected.
lock prototypelockWithPermit prototyperelock prototyperelockBatch prototype_createLockedPosition checks that user's expected bonusMultiplier_ is greater or equal to what was pulled from storage, and the one pulled from storage is used#2 - Ivshti
2022-01-16T02:57:09Z
agreed with the mitigation and lowering of severity
🌟 Selected for report: agusduha
Also found by: 0xsanson, Czar102, Dravee, GiveMeTestEther, WatchPug, p4st13r4, saian, sirhashalot
4.7941 USDC - $4.79
Czar102
Storage reads increase gas consumption.
MAX_TOTAL_XDEFI_SUPPLY is not declared as constant.
Consider declaring the variable as uint88 constant.
#0 - deluca-mike
2022-01-08T00:06:29Z
Yes, MAX_TOTAL_XDEFI_SUPPLY should have been a constant. In any case, due to changes resulting from other issues and recommendations, MAX_TOTAL_XDEFI_SUPPLY is being removed.
#1 - deluca-mike
2022-01-09T11:05:17Z
Duplicate #36
7.6096 USDC - $7.61
Czar102
Multiple storage changes increase gas consumption.
Current implementation changes storage variable bool _locked twice for every message call.
It is possible to make only a single state change by introducing a storage variable uint LOCKED_COUNTER and changing the implementation to the following.
modifier noReenter() { uint lockedCounter = LOCKED_COUNTER + 1; LOCKED_COUNTER = lockedCounter; _; require(lockedCounter == LOCKED_COUNTER, "REENTRY_NOT_ALLOWED"); }
#0 - deluca-mike
2022-01-08T01:43:14Z
We're going to move away from the bool and used this:
uint256 internal constant IS_NOT_LOCKED = uint256(1); uint256 internal constant IS_LOCKED = uint256(2); uint256 internal _lockedStatus = IS_NOT_LOCKED; modifier noReenterAndUpdateDistribution() { if (_lockedStatus == IS_LOCKED) revert NoReentering(); _lockedStatus = IS_LOCKED; _; _lockedStatus = IS_NOT_LOCKED; }
which should be cheaper.
#1 - deluca-mike
2022-01-09T11:05:04Z
Duplicate #1
🌟 Selected for report: Dravee
Also found by: Czar102, TomFrenchBlockchain, defsec
18.2673 USDC - $18.27
Czar102
Usage of memory arrays in, expecially as external functions' arguments, increases gas costs.
There are possible optimizations in XDEFIDistribution.sol in #77, #165, #186, #205, #320.
Consider changing external functions' memory arguments to calldata variables.
#0 - deluca-mike
2022-01-08T03:25:59Z
Agreed. We should have done this. Will change.
#1 - deluca-mike
2022-01-09T10:59:09Z
Duplicate #29