Platform: Code4rena
Start Date: 04/11/2021
Pot Size: $50,000 USDC
Total HM: 20
Participants: 28
Period: 7 days
Judge: 0xean
Total Solo HM: 11
Id: 51
League: ETH
Rank: 2/28
Findings: 8
Award: $7,101.60
🌟 Selected for report: 10
🚀 Solo Findings: 1
WatchPug
At L225 in _processWithdrawal(), it calls vestLock.vest() to vest 70% of the tokens bought.
However, PublicSale.sol contract never approve() mainToken to the vestLock contract, making _processWithdrawal() to revet at L225.
As a result, all the withdrawals will fail and all the funds will be frozen in the contract.
function _processWithdrawal (uint _era, uint _day, address _member) private returns (uint value) { uint memberUnits = mapEraDay_MemberUnits[_era][_day][_member]; // Get Member Units if (memberUnits == 0) { value = 0; // Do nothing if 0 (prevents revert) } else { value = getEmissionShare(_era, _day, _member); // Get the emission Share for Member mapEraDay_MemberUnits[_era][_day][_member] = 0; // Set to 0 since it will be withdrawn mapEraDay_UnitsRemaining[_era][_day] = mapEraDay_UnitsRemaining[_era][_day].sub(memberUnits); // Decrement Member Units mapEraDay_EmissionRemaining[_era][_day] = mapEraDay_EmissionRemaining[_era][_day].sub(value); // Decrement emission totalEmitted += value; // Add to Total Emitted uint256 v_value = value * 3 / 10; // Transfer 30%, lock the rest in vesting contract mainToken.transfer(_member, v_value); // ERC20 transfer function vestLock.vest(_member, value - v_value, 0); emit Withdrawal(msg.sender, _member, _era, _day, value, mapEraDay_EmissionRemaining[_era][_day]); } return value; }
function vest(address _beneficiary, uint256 _amount, uint256 _isRevocable) external payable whenNotPaused { require(_beneficiary != address(0), "Invalid address"); require( _amount > 0, "amount must be positive"); // require(totalVestedAmount.add(_amount) <= maxVestingAmount, 'maxVestingAmount is already vested'); require(_isRevocable == 0 || _isRevocable == 1, "revocable must be 0 or 1"); uint256 _unlockTimestamp = block.timestamp.add(unixYear); Timelock memory newVesting = Timelock(_amount, _unlockTimestamp); timelocks[_beneficiary].push(newVesting); if(_isRevocable == 0){ benRevocable[_beneficiary] = [false,false]; } else if(_isRevocable == 1){ benRevocable[_beneficiary] = [true,false]; } totalVestedAmount = totalVestedAmount.add(_amount); benTotal[_beneficiary] = benTotal[_beneficiary].add(_amount); // transfer to SC using delegate transfer // NOTE: the tokens has to be approved first by the caller to the SC using `approve()` method. vestingToken.transferFrom(msg.sender, address(this), _amount); emit TokenVested(_beneficiary, _amount, _unlockTimestamp, block.timestamp); }
Consider adding approve() in constructor().
_mainToken.approve(address(_vestLock), type(uint256).max);
#0 - chickenpie347
2022-01-04T01:36:07Z
Duplicate of #135
WatchPug
uint256 initialTargetPricePrecise = _getTargetPricePrecise(self); uint256 futureTargetPricePrecise = futureTargetPrice_.mul(TARGET_PRICE_PRECISION); if (futureTargetPricePrecise < initialTargetPricePrecise) { require( futureTargetPricePrecise.mul(MAX_RELATIVE_PRICE_CHANGE).div(WEI_UNIT) >= initialTargetPricePrecise, "futureTargetPrice_ is too small" ); } else { require( futureTargetPricePrecise <= initialTargetPricePrecise.mul(MAX_RELATIVE_PRICE_CHANGE).div(WEI_UNIT), "futureTargetPrice_ is too large" ); } self.initialTargetPrice = initialTargetPricePrecise; self.futureTargetPrice = futureTargetPricePrecise; self.initialTargetPriceTime = block.timestamp; self.futureTargetPriceTime = futureTime_;
At L1571-1581, it compares futureTargetPricePrecise with initialTargetPricePrecise to make sure the change is within the MAX_RELATIVE_PRICE_CHANGE (a constant of 1%).
However, the current implementation is wrong and it actually compares 1% of futureTargetPricePrecise to initialTargetPricePrecise when futureTargetPricePrecise < initialTargetPricePrecise and futureTargetPricePrecise to 1% of initialTargetPricePrecise when futureTargetPricePrecise >= initialTargetPricePrecise.
This will always fail, making it impossible to change the target price.
Change to:
if (futureTargetPricePrecise < initialTargetPricePrecise) { require( initialTargetPricePrecise.sub(futureTargetPricePrecise).mul(WEI_UNIT) <= initialTargetPricePrecise.mul(MAX_RELATIVE_PRICE_CHANGE), "futureTargetPrice_ is too small" ); } else { require( futureTargetPricePrecise.sub(initialTargetPricePrecise).mul(WEI_UNIT) <= initialTargetPricePrecise.mul(MAX_RELATIVE_PRICE_CHANGE), "futureTargetPrice_ is too large" ); }
#0 - chickenpie347
2022-01-04T01:28:04Z
Duplicate of #143
🌟 Selected for report: WatchPug
2907.6165 USDC - $2,907.62
WatchPug
Based on the context, the tokenPrecisionMultipliers used in price calculation should be calculated in realtime based on initialTargetPrice, futureTargetPrice, futureTargetPriceTime and current time, just like getA() and getA2().
However, in the current implementation, tokenPrecisionMultipliers used in price calculation is the stored value, it will only be changed when the owner called rampTargetPrice() and stopRampTargetPrice().
As a result, the targetPrice set by the owner will not be effective until another targetPrice is being set or stopRampTargetPrice() is called.
Consider adding Swap.targetPrice and changing the _xp() at L661 from:
function _xp(Swap storage self, uint256[] memory balances) internal view returns (uint256[] memory) { return _xp(balances, self.tokenPrecisionMultipliers); }
To:
function _xp(Swap storage self, uint256[] memory balances) internal view returns (uint256[] memory) { uint256[2] memory tokenPrecisionMultipliers = self.tokenPrecisionMultipliers; tokenPrecisionMultipliers[0] = self.targetPrice.originalPrecisionMultipliers[0].mul(_getTargetPricePrecise(self)).div(WEI_UNIT) return _xp(balances, tokenPrecisionMultipliers); }
WatchPug
function claim() external whenNotPaused nonReentrant { require(benRevocable[msg.sender][1] == false, 'Account must not already be revoked.'); uint256 amount = _claimableAmount(msg.sender).sub(benClaimed[msg.sender]); require(amount > 0, "Claimable amount must be positive"); require(amount <= benTotal[msg.sender], "Cannot withdraw more than total vested amount"); // transfer from SC benClaimed[msg.sender] = benClaimed[msg.sender].add(amount); totalClaimedAmount = totalClaimedAmount.add(amount); vestingToken.safeTransfer(msg.sender, amount); emit TokenClaimed(msg.sender, amount, block.timestamp); }
At L195, function claim() will call function _claimableAmount(), which includes an unbounded for loop.
function _claimableAmount(address _addr) private returns (uint256) { uint256 completely_vested = 0; uint256 partial_sum = 0; uint256 inc = 0; // iterate across all the vestings // & check if the releaseTimestamp is elapsed // then, add all the amounts as claimable amount for (uint256 i = benVestingIndex[_addr]; i < timelocks[_addr].length; i++) { if (block.timestamp >= timelocks[_addr][i].releaseTimestamp) { inc += 1; completely_vested = completely_vested.add(timelocks[_addr][i].amount); } else { uint256 iTimeStamp = timelocks[_addr][i].releaseTimestamp.sub(unixYear); uint256 claimable = block.timestamp.sub(iTimeStamp).mul(timelocks[_addr][i].amount).div(unixYear); partial_sum = partial_sum.add(claimable); } } benVestingIndex[_addr] +=inc; benVested[_addr][0] = benVested[_addr][0].add(completely_vested); benVested[_addr][1] = partial_sum; uint256 s = benVested[_addr][0].add(partial_sum); assert(s <= benTotal[_addr]); return s; }
An attacker can call function vest() and add a lot of very small amounts of new vestings to the user, if the length of the user's vestings is large enough, the gas cost of function claim() can exceed the block limit, making it impossible for the user to claim.
Essentially, this allows an attacker to freeze (or burn, considering that the contract is not upgradable) the unclaimed funds of an arbitrary user.
Consider allowing users to claim() a specific range of vestings indexes.
#0 - chickenpie347
2022-01-04T01:37:06Z
Duplicate of #120
WatchPug
Calling ERC20.transfer() without handling the returned value is unsafe.
function _processWithdrawal (uint _era, uint _day, address _member) private returns (uint value) { uint memberUnits = mapEraDay_MemberUnits[_era][_day][_member]; // Get Member Units if (memberUnits == 0) { value = 0; // Do nothing if 0 (prevents revert) } else { value = getEmissionShare(_era, _day, _member); // Get the emission Share for Member mapEraDay_MemberUnits[_era][_day][_member] = 0; // Set to 0 since it will be withdrawn mapEraDay_UnitsRemaining[_era][_day] = mapEraDay_UnitsRemaining[_era][_day].sub(memberUnits); // Decrement Member Units mapEraDay_EmissionRemaining[_era][_day] = mapEraDay_EmissionRemaining[_era][_day].sub(value); // Decrement emission totalEmitted += value; // Add to Total Emitted uint256 v_value = value * 3 / 10; // Transfer 30%, lock the rest in vesting contract mainToken.transfer(_member, v_value); // ERC20 transfer function vestLock.vest(_member, value - v_value, 0); emit Withdrawal(msg.sender, _member, _era, _day, value, mapEraDay_EmissionRemaining[_era][_day]); } return value; }
Consider using OpenZeppelin's SafeERC20 library with safe versions of transfer functions.
#0 - chickenpie347
2022-01-04T01:28:36Z
Duplicate of #31
WatchPug
vest() can be called by anyone with an arbitrary _beneficiary address to add a Timelock (vesting) to the _beneficiary.
At L83-88, it changes the global storage of revokable settings for the _beneficiary.
This allows anyone to change the revokable settings for other users. Non-revocable vestings can later be changed into revokable, and then be revoked, causing the user to lose funds.
function vest(address _beneficiary, uint256 _amount, uint256 _isRevocable) external payable whenNotPaused { require(_beneficiary != address(0), "Invalid address"); require( _amount > 0, "amount must be positive"); // require(totalVestedAmount.add(_amount) <= maxVestingAmount, 'maxVestingAmount is already vested'); require(_isRevocable == 0 || _isRevocable == 1, "revocable must be 0 or 1"); uint256 _unlockTimestamp = block.timestamp.add(unixYear); Timelock memory newVesting = Timelock(_amount, _unlockTimestamp); timelocks[_beneficiary].push(newVesting); if(_isRevocable == 0){ benRevocable[_beneficiary] = [false,false]; } else if(_isRevocable == 1){ benRevocable[_beneficiary] = [true,false]; } totalVestedAmount = totalVestedAmount.add(_amount); benTotal[_beneficiary] = benTotal[_beneficiary].add(_amount); // transfer to SC using delegate transfer // NOTE: the tokens has to be approved first by the caller to the SC using `approve()` method. vestingToken.transferFrom(msg.sender, address(this), _amount); emit TokenVested(_beneficiary, _amount, _unlockTimestamp, block.timestamp); }
Consider making the revokable settings set per Timelock instead of per address.
#0 - chickenpie347
2021-11-16T13:53:33Z
Addressed in #132.
🌟 Selected for report: WatchPug
290.7617 USDC - $290.76
WatchPug
In Swap.constructor(), when adding tokens with decimals larger than 18, it reverts at L177-179.
targetPriceStorage.originalPrecisionMultipliers[i] = 10 ** uint256(SwapUtils.POOL_PRECISION_DECIMALS).sub( uint256(decimals[i]) );
// the precision all pools tokens will be converted to uint8 public constant POOL_PRECISION_DECIMALS = 18;
#0 - chickenpie347
2022-01-04T01:15:59Z
It is unlikely that a token with larger than 18 decimals would be created to be added to the pool. 18 decimals is the ERC20 standard, and all other notable tokens use decimals less than 18, I personally have yet to come across a token with more than 18 decimals that is significant enough to optimise for.
#1 - 0xean
2022-01-08T03:11:56Z
Leaving as low risk. ERC20 does not have a standard - https://eips.ethereum.org/EIPS/eip-20
🌟 Selected for report: WatchPug
290.7617 USDC - $290.76
WatchPug
There are 4 events with the parameter lpTokenSupply in SwapUtils.sol, but the value of lpTokenSupply is not consistent.
For the event RemoveLiquidityOne, lpTokenSupply is post burn:
emit RemoveLiquidity(msg.sender, amounts, self.lpToken.totalSupply());
For the event RemoveLiquidityOne, lpTokenSupply is pre burn:
For the event removeLiquidityImbalance, lpTokenSupply is post burn:
For the event AddLiquidity, lpTokenSupply is post mint:
Given that 3 out of the 4 events are using updated totalSupply as lpTokenSupply, consider changing RemoveLiquidityOne to post burn totalSupply.
WatchPug
For uint256, they will always >= 0, therefore check if they >= 0 is redundant.
require(_a >= 0 && _a <= SwapUtils.MAX_A, "_a not within the limits"); require(_a2 >= 0 && _a2 <= SwapUtils.MAX_A, "_a2 not within the limits");
require( futureA_ >= 0 && futureA_ <= MAX_A, "futureA_ must be >= 0 and <= MAX_A" );
require( futureA2_ >= 0 && futureA2_ <= MAX_A, "futureA2_ must be >= 0 and <= MAX_A" );
require( futureTargetPrice_ >= 0, "futureTargetPrice_ must be >= 0" );
#0 - chickenpie347
2022-01-04T01:23:07Z
Duplicate of #133
WatchPug
uint256 private unixYear = 52 * 7 * 24 * 60 * 60;
Some storage variables include unixYear will not never be changed and they should not be.
Changing them to constant can save gas.
Change to:
uint256 constant UNIX_YEAR = 52 * 7 * 24 * 60 * 60;
#0 - chickenpie347
2022-01-04T01:40:13Z
Duplicate of #136
🌟 Selected for report: TomFrenchBlockchain
Also found by: PranavG, Reigada, WatchPug, jah, nathaniel, pants, pauliax, pmerkleplant
2.147 USDC - $2.15
WatchPug
Some storage variables include vestingToken and multiSig are unnecessary as they will never be changed.
Change to immutable can save gas.
#0 - 0xean
2022-01-08T22:43:30Z
duplicate of #1
🌟 Selected for report: WatchPug
44.8876 USDC - $44.89
WatchPug
transferredDx is unnecessary, it can be replaced with dx.
function swap( Swap storage self, uint8 tokenIndexFrom, uint8 tokenIndexTo, uint256 dx, uint256 minDy ) external returns (uint256) { require( dx <= self.pooledTokens[tokenIndexFrom].balanceOf(msg.sender), "Cannot swap more than you own" ); // Transfer tokens first to see if a fee was charged on transfer uint256 beforeBalance = self.pooledTokens[tokenIndexFrom].balanceOf(address(this)); self.pooledTokens[tokenIndexFrom].safeTransferFrom( msg.sender, address(this), dx ); // Use the actual transferred amount for AMM math uint256 transferredDx = self.pooledTokens[tokenIndexFrom].balanceOf(address(this)).sub( beforeBalance ); // ...
Change to:
// Use the actual transferred amount for AMM math uint256 dx = self.pooledTokens[tokenIndexFrom].balanceOf(address(this)).sub( beforeBalance ); // ...
🌟 Selected for report: WatchPug
44.8876 USDC - $44.89
WatchPug
getYD() already require(tokenIndex < numTokens, "..."), so the check in getYDC() is redundant.
Removing it will make the code simpler and save some gas.
function getYDC( Swap storage self, uint256 a, uint8 tokenIndex, uint256[] memory xp, uint256 d ) internal view returns (uint256) { uint256 numTokens = xp.length; require(tokenIndex < numTokens, "Token not found"); // calculate y uint256 y = getYD(a, tokenIndex, xp, d); // ... }
function getYD( uint256 a, uint8 tokenIndex, uint256[] memory xp, uint256 d ) internal pure returns (uint256) { uint256 numTokens = xp.length; require(tokenIndex < numTokens, "Token not found"); // ... }
Remove the redundant code.
🌟 Selected for report: WatchPug
44.8876 USDC - $44.89
WatchPug
function removeLiquidityOneToken( Swap storage self, uint256 tokenAmount, uint8 tokenIndex, uint256 minAmount ) external returns (uint256) { uint256 totalSupply = self.lpToken.totalSupply(); uint256 numTokens = self.pooledTokens.length; require( tokenAmount <= self.lpToken.balanceOf(msg.sender), ">LP.balanceOf" ); require(tokenIndex < numTokens, "Token not found");
The external call to get the totalSupply of the lpToken can be done later to avoid unnecessary code execution when the check of tokenAmount and tokenIndex does not pass.
20.1994 USDC - $20.20
WatchPug
function getD(Swap storage self) internal view returns (uint256) { uint256 a = determineA(self, _xp(self)); // determine the correct A return getD(_xp(self), a); }
a is unnecessary as it's being used only once. The result of _xp(self) can be cached to avoid calling it twice.
Change to:
function getD(Swap storage self) internal view returns (uint256) { uint256[] memory xp = _xp(self); return getD(xp, determineA(self, xp)); }
WatchPug
At L149-150, _pooledTokens.length and decimals.length are both required to be equal to 2.
Therefore, require(_pooledTokens.length == decimals.length, "..."); is redundant.
Removing it will make the code simpler and save some gas.
require(_pooledTokens.length == 2, "_pooledTokens.length must be 2 in length"); require(decimals.length == 2, "decimals.length must be 2 in length"); require( _pooledTokens.length == decimals.length, "_pooledTokens decimals mismatch" );
Remove the redundant check.
#0 - chickenpie347
2022-01-04T01:21:44Z
Duplicate of #14
#1 - 0xean
2022-01-09T00:36:12Z
Duplicate of #241
not #14
2.6837 USDC - $2.68
WatchPug
The current design requires the number of pooledTokens to be 2, therefore pooledTokens.length can be replaced with literal 2 to save ~100 gas from each storage read (SLOAD after Berlin).
Instances include:
20.1994 USDC - $20.20
WatchPug
Every call to an external contract costs a decent amount of gas. For optimization of gas usage, external call results should be cached if they are being used for more than one time.
For example:
self.lpToken.totalSupply() can be cached.🌟 Selected for report: WatchPug
44.8876 USDC - $44.89
WatchPug
benVested[_addr][1] = partial_sum;
benVested[_addr][1] is never used in the contract and the sum of partial claimable vesting is changing every second. Removing it can save gas.
WatchPug
msg.sender wont be address(0).
require(msg.sender != address(0));
#0 - chickenpie347
2022-01-04T01:40:53Z
Duplicate of #24
WatchPug
totalVestedAmount = 0; totalClaimedAmount = 0;
Setting uint256 variables to 0 is redundant as they default to 0.
#0 - chickenpie347
2022-01-04T01:48:10Z
Duplicate of #5
WatchPug
For the storage variables that will be accessed multiple times, cache and read from the stack can save ~100 gas from each extra read (SLOAD after Berlin).
For example:
function swap( Swap storage self, uint8 tokenIndexFrom, uint8 tokenIndexTo, uint256 dx, uint256 minDy ) external returns (uint256) { require( dx <= self.pooledTokens[tokenIndexFrom].balanceOf(msg.sender), "Cannot swap more than you own" ); // Transfer tokens first to see if a fee was charged on transfer uint256 beforeBalance = self.pooledTokens[tokenIndexFrom].balanceOf(address(this)); self.pooledTokens[tokenIndexFrom].safeTransferFrom( msg.sender, address(this), dx ); // Use the actual transferred amount for AMM math uint256 transferredDx = self.pooledTokens[tokenIndexFrom].balanceOf(address(this)).sub( beforeBalance ); // ... }
self.pooledTokens[tokenIndexFrom] can be cached.
Change to:
function swap( Swap storage self, uint8 tokenIndexFrom, uint8 tokenIndexTo, uint256 dx, uint256 minDy ) external returns (uint256) { IERC20 tokenFrom = self.pooledTokens[tokenIndexFrom]; require( dx <= tokenFrom.balanceOf(msg.sender), "Cannot swap more than you own" ); // Transfer tokens first to see if a fee was charged on transfer uint256 beforeBalance = tokenFrom.balanceOf(address(this)); tokenFrom.safeTransferFrom(msg.sender, address(this), dx); // Use the actual transferred amount for AMM math uint256 transferredDx = tokenFrom.balanceOf(address(this)).sub(beforeBalance); // ... }
#0 - chickenpie347
2022-01-04T01:17:01Z
Duplicate of #13
20.1994 USDC - $20.20
WatchPug
emission in if (currentDay >= daysPerEra) {...} is only used for storing the result of getNextEraEmission() temporarily, use a local variable instead will be much cheaper.
if (currentDay >= daysPerEra) { // If time passed the next Era time currentEra += 1; currentDay = 0; // Increment Era, reset Day nextEraTime = _now + (secondsPerDay * daysPerEra); // Set next Era time emission = getNextEraEmission(); // Get correct emission mapEra_Emission[currentEra] = emission; // Map emission to Era emit NewEra(currentEra, emission, nextEraTime, totalBurnt); // Emit Event } currentDay += 1; // Increment Day nextDayTime = _now + secondsPerDay; // Set next Day time emission = getDayEmission();
Change to:
if (currentDay >= daysPerEra) { // If time passed the next Era time currentEra += 1; currentDay = 0; // Increment Era, reset Day nextEraTime = _now + (secondsPerDay * daysPerEra); // Set next Era time uint256 _nextEraEmission = getNextEraEmission(); // Get correct emission mapEra_Emission[currentEra] = _nextEraEmission; // Map emission to Era emit NewEra(currentEra, _nextEraEmission, nextEraTime, totalBurnt); // Emit Event } currentDay += 1; // Increment Day nextDayTime = _now + secondsPerDay; // Set next Day time emission = getDayEmission();
#0 - chickenpie347
2022-01-04T01:32:55Z
Duplicate of #117
12.1196 USDC - $12.12
WatchPug
Every reason string takes at least 32 bytes.
Use short reason strings that fits in 32 bytes or it will become more expensive.
Instances include:
require(_pooledTokens.length == 2, "_pooledTokens.length must be 2 in length"); require(decimals.length == 2, "decimals.length must be 2 in length");
require(amount > 0, "Claimable amount must be positive"); require(amount <= benTotal[msg.sender], "Cannot withdraw more than total vested amount");
require( futureA2_ >= 0 && futureA2_ <= MAX_A, "futureA2_ must be >= 0 and <= MAX_A" );
#0 - chickenpie347
2022-01-04T01:22:04Z
Duplicate of #23
12.1196 USDC - $12.12
WatchPug
value is unused.
function getEmissionShare(uint era, uint day, address member) public view returns (uint value) { uint memberUnits = mapEraDay_MemberUnits[era][day][member]; // Get Member Units if (memberUnits == 0) { return 0; // If 0, return 0 } else { uint totalUnits = mapEraDay_UnitsRemaining[era][day]; // Get Total Units uint emissionRemaining = mapEraDay_EmissionRemaining[era][day]; // Get emission remaining for Day uint balance = mainToken.balanceOf(address(this)); if (emissionRemaining > balance) { emissionRemaining = balance; // In case less than required emission } return (emissionRemaining * memberUnits) / totalUnits; // Calculate share } }
#0 - chickenpie347
2022-01-04T01:24:36Z
Duplicate of #25
20.1994 USDC - $20.20
WatchPug
The following source units are imported but not referenced in the contract:
import "./hardhat/console.sol";
Check all imports and remove all unused/unreferenced and unnecessary imports.
#0 - chickenpie347
2022-01-04T01:21:09Z
Duplicate of #173
WatchPug
Reading array length at each iteration of the loop takes 6 gas (3 for MLOAD and 3 to place memory_offset) in the stack.
Caching the array length in the stack saves around 3 gas per iteration.
Instances include:
SwapUtils.sol#calculateWithdrawOneTokenDY()
SwapUtils.sol#_calculateRemoveLiquidity()
SwapUtils.sol#removeLiquidityImbalance()
SwapUtils.sol#withdrawAdminFees()
SwapUtils.sol#removeLiquidity()
#0 - chickenpie347
2022-01-04T01:31:43Z
Duplicate of #6 , #13 , #14