Platform: Code4rena
Start Date: 21/06/2022
Pot Size: $50,000 USDC
Total HM: 31
Participants: 99
Period: 5 days
Judges: moose-code, JasoonS, denhampreen
Total Solo HM: 17
Id: 139
League: ETH
Rank: 7/99
Findings: 4
Award: $2,433.19
🌟 Selected for report: 2
🚀 Solo Findings: 1
🌟 Selected for report: unforgiven
Also found by: IllIllI, TrungOre, asutorufos, hake, robee
545.2654 USDC - $545.27
One of the tokens supported by this project is USDC, which is an upgradeable contract, and the code specifically casts addresses to IERC20Upgradeable rather than to IERC20, so the intention is for the code to support upgrades. If USDC ever upgrades to have a fee-on-transfer, rebasing behavior, or some other non-standard behavior, the contracts in this project will break, locking user funds in the state that they were before the upgrade.
For example, if an upgrade starts fee-on-transfer behavior, this function will revert, because it assumes _amount is available once claimeWithdraw() has happened, which will not be the case if a fee is charged:
File: src/contracts/LiquidityReserve.sol #X 188 function instantUnstake(uint256 _amount, address _recipient) 189 external 190 onlyStakingContract 191 { 192 require(isReserveEnabled, "Not enabled yet"); 193 // claim the stakingToken from previous unstakes 194 IStaking(stakingContract).claimWithdraw(address(this)); 195 196 uint256 amountMinusFee = _amount - ((_amount * fee) / BASIS_POINTS); 197 198 IERC20Upgradeable(rewardToken).safeTransferFrom( 199 msg.sender, 200 address(this), 201 _amount 202 ); 203 204 IERC20Upgradeable(stakingToken).safeTransfer( 205 _recipient, 206 amountMinusFee 207 ); 208 unstakeAllRewardTokens(); 209 }
There are a lot of other examples where the balances aren't checked
Code inspection
Measure balances before and after transfers, and use the difference as the amount rather than the stated amount.
#0 - toshiSat
2022-07-27T22:36:06Z
We will update the documentation to include these contracts will not work for ERC20 tokens that have fees
#1 - KenzoAgada
2022-08-26T09:02:58Z
In the judging sheet this was judged as unique, but looks like a duplicate of M-17 #222
🌟 Selected for report: IllIllI
1211.7009 USDC - $1,211.70
Users may be unable to withdraw/remove their liquidity from the LiquidityReserve if a user decides to grief the contract.
This is the only function in this contract that is able to unstake funds, so that they can be withdrawn/removed:
File: src/contracts/LiquidityReserve.sol #1 214 function unstakeAllRewardTokens() public { 215 require(isReserveEnabled, "Not enabled yet"); 216 uint256 coolDownAmount = IStaking(stakingContract) 217 .coolDownInfo(address(this)) 218 .amount; 219 if (coolDownAmount == 0) { 220 uint256 amount = IERC20Upgradeable(rewardToken).balanceOf( 221 address(this) 222 ); 223 if (amount > 0) IStaking(stakingContract).unstake(amount, false); 224 } 225 }
The function requires that the coolDownAmount is zero, or else it skips the unstake() call. A malicious user can make coolDownAmount non-zero by calling Staking.instantUnstakeReserve() when the previous reward is claimed, with just a large enough amount to satisfy the transfer of the amount and of the fee, so there is essentially zero left for other users to withdraw. The function calls LiquidityReserve.instantUnstake():
File: src/contracts/Staking.sol #2 571 function instantUnstakeReserve(uint256 _amount) external { 572 require(_amount > 0, "Invalid amount"); 573 // prevent unstaking if override due to vulnerabilities 574 require( 575 !isUnstakingPaused && !isInstantUnstakingPaused, 576 "Unstaking is paused" 577 ); 578 579 rebase(); 580 _retrieveBalanceFromUser(_amount, msg.sender); 581 582 uint256 reserveBalance = IERC20Upgradeable(STAKING_TOKEN).balanceOf( 583 LIQUIDITY_RESERVE 584 ); 585 586 require(reserveBalance >= _amount, "Not enough funds in reserve"); 587 588 ILiquidityReserve(LIQUIDITY_RESERVE).instantUnstake( 589 _amount, 590 msg.sender 591 ); 592 }
Which boosts the cooldown amount above zero in its call to unstakeAllRewardTokens() and then IStaking.unstake():
File: src/contracts/LiquidityReserve.sol #3 188 function instantUnstake(uint256 _amount, address _recipient) 189 external 190 onlyStakingContract 191 { 192 require(isReserveEnabled, "Not enabled yet"); 193 // claim the stakingToken from previous unstakes 194 IStaking(stakingContract).claimWithdraw(address(this)); 195 196 uint256 amountMinusFee = _amount - ((_amount * fee) / BASIS_POINTS); 197 198 IERC20Upgradeable(rewardToken).safeTransferFrom( 199 msg.sender, 200 address(this), 201 _amount 202 ); 203 204 IERC20Upgradeable(stakingToken).safeTransfer( 205 _recipient, 206 amountMinusFee 207 ); 208 unstakeAllRewardTokens(); 209 } 210 211 /** 212 @notice find balance of reward tokens in contract and unstake them from staking contract 213 */ 214 function unstakeAllRewardTokens() public { 215 require(isReserveEnabled, "Not enabled yet"); 216 uint256 coolDownAmount = IStaking(stakingContract) 217 .coolDownInfo(address(this)) 218 .amount; 219 if (coolDownAmount == 0) { 220 uint256 amount = IERC20Upgradeable(rewardToken).balanceOf( 221 address(this) 222 ); 223 if (amount > 0) IStaking(stakingContract).unstake(amount, false); 224 } 225 }
File: src/contracts/Staking.sol #4 674 function unstake(uint256 _amount, bool _trigger) external { 675 // prevent unstaking if override due to vulnerabilities asdf 676 require(!isUnstakingPaused, "Unstaking is paused"); 677 if (_trigger) { 678 rebase(); 679 } 680 _retrieveBalanceFromUser(_amount, msg.sender); 681 682 Claim storage userCoolInfo = coolDownInfo[msg.sender]; 683 684 // try to claim withdraw if user has withdraws to claim function will check if withdraw is valid 685 claimWithdraw(msg.sender); 686 687 coolDownInfo[msg.sender] = Claim({ 688 amount: userCoolInfo.amount + _amount, 689 credits: userCoolInfo.credits + 690 IYieldy(YIELDY_TOKEN).creditsForTokenBalance(_amount), 691 expiry: epoch.number + coolDownPeriod 692 });
If the malicious user is a miner, that miner can make sure that the block where the previous cooldown expires and is claimed, is the same block where the miner griefs by doing an instant unstake of a small amount, preventing larger amounts from going through. Until the miner decides to stop this behavior, funds will be locked in the contract.
Code inspection
Keep track of submitted amounts during the cooldown, and batch-submit them during the next open window, rather than making it first-come-first-served
#0 - 0xean
2022-06-27T21:52:09Z
The warden does identify a potential attack, but the assumptions that are being made for it to work are pretty hard to imagine. For one, It would require a miner to always be able to process a specific block in order to continually DOS the contract. This is very infeasible.
Additionally, if this scenario was to occur, we could pause instant unstaking and wait for the cooldown to expire in order to retrieve funds. The ability to toggle the instant unstake negates the call path the warden has suggested since Staking.instantUnstakeReserve() would revert.
Given all of this, I would put this entire attack vector as super low risk and suggest its downgraded to QA.
#1 - toshiSat
2022-06-27T21:53:38Z
sponsor dispute severity: QA
#2 - JasoonS
2022-07-30T15:24:55Z
I'm going to make this a Medium. While I agree the assumptions are out of the imaginable in reality, it is something that should be looked into for the contracts more seriously than the typical QA
🌟 Selected for report: IllIllI
Also found by: 0x1337, 0x1f8b, 0x29A, 0x52, 0xDjango, 0xNazgul, 0xNineDec, 0xc0ffEE, 0xf15ers, 0xmint, Bnke0x0, BowTiedWardens, Chom, ElKu, FudgyDRS, Funen, GalloDaSballo, GimelSec, JC, Kaiziron, Lambda, Limbooo, Metatron, MiloTruck, Noah3o6, Picodes, PumpkingWok, PwnedNoMore, Sm4rty, StErMi, TomJ, TrungOre, UnusualTurtle, Waze, _Adam, aga7hokakological, ak1, antonttc, berndartmueller, cccz, cryptphi, csanuragjain, defsec, delfin454000, dipp, elprofesor, exd0tpy, fatherOfBlocks, hake, hansfriese, hubble, joestakey, kenta, ladboy233, mics, oyc_109, pashov, pedr02b2, reassor, robee, samruna, scaraven, shung, sikorico, simon135, sseefried, tchkvsky, unforgiven, zzzitron
520.1657 USDC - $520.17
| Issue | Instances | |
|---|---|---|
| 1 | Batch-related functions will revert if removeAddress() is called | 2 |
| 2 | Staking contract's token not verified to be the same token as the staking token | 1 |
| 3 | Missing infinite approval functionality | 1 |
| 4 | Missing checks that the end time matches the duration | 1 |
| 5 | Missing input validations and timelocks | 5 |
| 6 | Front-runable initializer | 2 |
Total: 12 instances over 6 issues
| Issue | Instances | |
|---|---|---|
| 1 | Return values of approve() not checked | 2 |
| 2 | Misleading variable names | 1 |
| 3 | public functions not called by the contract should be declared external instead | 1 |
| 4 | constants should be defined rather than using magic numbers | 3 |
| 5 | Use a more recent version of solidity | 3 |
| 6 | Typos | 10 |
| 7 | NatSpec is incomplete | 2 |
| 8 | Event is missing indexed fields | 12 |
Total: 34 instances over 8 issues
removeAddress() is calledremoveAddress() removes entries from the storage array that holds batch contract addresses, but it doesn't fill in the deleted entries with a replacement value. When other functions hit these null entries and try to call functions on the zero address, they'll revert, causing the whole function to fail
There are 2 instances of this issue:
File: src/contracts/BatchRequests.sol #1 33 function canBatchContracts() external view returns (Batch[] memory) { 34 uint256 contractsLength = contracts.length; 35 Batch[] memory batch = new Batch[](contractsLength); 36 for (uint256 i; i < contractsLength; ) { 37: bool canBatch = IStaking(contracts[i]).canBatchTransactions();
File: src/contracts/BatchRequests.sol #2 50 function canBatchContractByIndex(uint256 _index) 51 external 52 view 53 returns (address, bool) 54 { 55 return ( 56 contracts[_index], 57: IStaking(contracts[_index]).canBatchTransactions()
There may be a mismatch between the token that _stakingContract is in charge of, and the actual token used by the LiquidityReserve. This code should check that they are in fact the same
There is 1 instance of this issue:
File: src/contracts/LiquidityReserve.sol #1 57 function enableLiquidityReserve(address _stakingContract) 58 external 59 onlyOwner 60 { 61 require(!isReserveEnabled, "Already enabled"); 62 require(_stakingContract != address(0), "Invalid address"); 63 64 uint256 stakingTokenBalance = IERC20Upgradeable(stakingToken).balanceOf( 65 msg.sender 66 ); 67 // require address has minimum liquidity 68 require( 69 stakingTokenBalance >= MINIMUM_LIQUIDITY, 70 "Not enough staking tokens" 71 ); 72: stakingContract = _stakingContract;
Most other contracts in the repository use type(uint256).max to mean infinite approval, rather than a specific approval amount. Not doing the same thing here will mean inconsistent behavior between the components, will mean that approvals will eventually run down to zero, and will mean that there will be hard-to-track-down issues when things eventually start failing
There is 1 instance of this issue:
File: src/contracts/Yieldy.sol #1 210 require(_allowances[_from][msg.sender] >= _value, "Allowance too low"); 211 212 uint256 newValue = _allowances[_from][msg.sender] - _value; 213 _allowances[_from][msg.sender] = newValue; 214: emit Approval(_from, msg.sender, newValue);
There is 1 instance of this issue:
File: src/contracts/Staking.sol #1 95 duration: _epochDuration, 96 number: 1, 97 timestamp: block.timestamp, // we know about the issues surrounding block.timestamp, using it here will not cause any problems 98: endTime: _firstEpochEndTime,
The following instances are missing checks for zero addresses and or valid ranges for values. Even if the DAO is the one setting these values, it's important to add sanity checks in case someone does a fat-finger operation that is missed by DAO participants who may not be very technical. There are also no timelocks involved, which should be rectified
There are 5 instances of this issue:
File: src/contracts/Staking.sol 217 function setEpochDuration(uint256 duration) external onlyOwner { 218 epoch.duration = duration; 219 emit LogSetEpochDuration(block.number, duration); 220: } 167 function setAffiliateFee(uint256 _affiliateFee) external onlyOwner { 168 affiliateFee = _affiliateFee; 169 emit LogSetAffiliateFee(block.number, _affiliateFee); 170: } 217 function setEpochDuration(uint256 duration) external onlyOwner { 218 epoch.duration = duration; 219 emit LogSetEpochDuration(block.number, duration); 220: } 226 function setWarmUpPeriod(uint256 _vestingPeriod) external onlyOwner { 227 warmUpPeriod = _vestingPeriod; 228 emit LogSetWarmUpPeriod(block.number, _vestingPeriod); 229: } 235 function setCoolDownPeriod(uint256 _vestingPeriod) external onlyOwner { 236 coolDownPeriod = _vestingPeriod; 237 emit LogSetCoolDownPeriod(block.number, _vestingPeriod); 238: }
There is nothing preventing another account from calling the initializer before the contract owner. In the best case, the owner is forced to waste gas and re-deploy. In the worst case, the owner does not notice that his/her call reverts, and everyone starts using a contract under the control of an attacker
There are 2 instances of this issue:
File: src/contracts/LiquidityReserve.sol #1 36 function initialize( 37 string memory _tokenName, 38 string memory _tokenSymbol, 39 address _stakingToken, 40 address _rewardToken 41: ) external initializer {
File: src/contracts/Yieldy.sol #2 29 function initialize( 30 string memory _tokenName, 31 string memory _tokenSymbol, 32 uint8 _decimal 33: ) external initializer {
approve() not checkedNot all IERC20 implementations revert() when there's a failure in approve(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything
There are 2 instances of this issue:
File: src/contracts/Staking.sol #1 79: IERC20(TOKE_POOL).approve(CURVE_POOL, type(uint256).max);
File: src/contracts/Staking.sol #2 83: IERC20(STAKING_TOKEN).approve(TOKE_POOL, type(uint256).max);
There is 1 instance of this issue:
File: src/contracts/LiquidityReserve.sol #1 /// @audit code is no longer FOX-specific 112: uint256 lrFoxSupply = totalSupply();
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
There is 1 instance of this issue:
File: src/contracts/Staking.sol #1 370: function unstakeAllFromTokemak() public onlyOwner {
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 3 instances of this issue:
File: src/contracts/Staking.sol #1 /// @audit 0x9008D19f58AAbD9eD0D60971565AA8510560ab41 73: COW_SETTLEMENT = 0x9008D19f58AAbD9eD0D60971565AA8510560ab41;
File: src/contracts/Staking.sol #2 /// @audit 0xC92E8bdf79f0507f65a392b0ab4667716BFE0110 74: COW_RELAYER = 0xC92E8bdf79f0507f65a392b0ab4667716BFE0110;
File: src/contracts/Staking.sol #3 /// @audit 12 76: timeLeftToRequestWithdrawal = 12 hours;
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
There are 3 instances of this issue:
File: src/contracts/LiquidityReserve.sol #1 2: pragma solidity 0.8.9;
File: src/contracts/Migration.sol #2 2: pragma solidity 0.8.9;
File: src/contracts/Staking.sol #3 2: pragma solidity 0.8.9;
There are 10 instances of this issue:
File: src/contracts/Staking.sol /// @audit withdrawRequest 315: @notice creates a withdrawRequest with Tokemak /// @audit requestedWithdraws 316: @dev requestedWithdraws take 1 tokemak cycle to be available for withdraw /// @audit cycleTime 348: @notice checks TOKE's cycleTime is within duration to batch the transactions /// @audit requestWithdraw 367: @notice owner function to requestWithdraw all FOX from tokemak in case of an attack on tokemak /// @audit requestWithdraw 368: @dev this bypasses the normal flow of sending a withdrawal request and allows the owner to requestWithdraw entire pool balance /// @audit asdf 675: // prevent unstaking if override due to vulnerabilities asdf /// @audit autoRebase 767: * @dev this is function is called from claimFromTokemak if the autoRebase bool is set to true
File: src/contracts/Yieldy.sol /// @audit profit_ 74: @notice increases Yieldy supply to increase staking balances relative to profit_ /// @audit co 232: @notice called from the staking contract co create Yieldy tokens /// @audit co 262: @notice called from the staking contract co burn Yieldy tokens
There are 2 instances of this issue:
File: src/contracts/BatchRequests.sol #1 /// @audit Missing: '@param _index' 46 /** 47 @notice shows if contracts can batch by index 48 @return (address, bool) 49 */ 50 function canBatchContractByIndex(uint256 _index) 51 external 52 view 53: returns (address, bool)
File: src/contracts/BatchRequests.sol #2 /// @audit Missing: '@param _index' 61 /** 62 @notice get address in contracts by index 63 @return address 64 */ 65: function getAddressByIndex(uint256 _index) external view returns (address) {
indexed fieldsEach event should use three indexed fields if there are three or more fields
There are 12 instances of this issue:
File: src/contracts/LiquidityReserve.sol 21: event FeeChanged(uint256 fee);
File: src/contracts/Staking.sol 21: event LogSetEpochDuration(uint256 indexed blockNumber, uint256 duration); 22: event LogSetWarmUpPeriod(uint256 indexed blockNumber, uint256 period); 23: event LogSetCoolDownPeriod(uint256 indexed blockNumber, uint256 period); 24: event LogSetPauseStaking(uint256 indexed blockNumber, bool shouldPause); 25: event LogSetPauseUnstaking(uint256 indexed blockNumber, bool shouldPause); 26 event LogSetPauseInstantUnstaking( 27 uint256 indexed blockNumber, 28 bool shouldPause 29: ); 30 event LogSetAffiliateAddress( 31 uint256 indexed blockNumber, 32 address affilateAddress 33: ); 34: event LogSetAffiliateFee(uint256 indexed blockNumber, uint256 fee); 36: event LogSetCurvePool(address indexed curvePool, int128 to, int128 from);
File: src/contracts/Yieldy.sol 15 event LogSupply( 16 uint256 indexed epoch, 17 uint256 timestamp, 18 uint256 totalSupply 19: ); 21: event LogRebase(uint256 indexed epoch, uint256 rebase, uint256 index);
#0 - moose-code
2022-09-05T10:27:14Z
Low risk issues: 1 - Agree 2 - Agree 3 - Agree 4 - Agree 5 - Agree 6 -Agree
Informational: 1- Agree 2 -Agree 3 - Agree 4 - Strongly agree, this is very helpful. 5 - Agree 6 - Agree 7 - Agree 8 - Agree
🌟 Selected for report: BowTiedWardens
Also found by: 0v3rf10w, 0x1f8b, 0x29A, 0xKitsune, 0xNazgul, 0xf15ers, 0xkatana, 0xmint, 8olidity, ACai, Bnke0x0, Chom, ElKu, Fabble, Fitraldys, FudgyDRS, Funen, GalloDaSballo, GimelSec, IllIllI, JC, Kaiziron, Lambda, Limbooo, MiloTruck, Noah3o6, Nyamcil, Picodes, PwnedNoMore, Randyyy, RedOneN, Sm4rty, StErMi, TomJ, Tomio, TrungOre, UnusualTurtle, Waze, _Adam, aga7hokakological, ajtra, antonttc, asutorufos, bardamu, c3phas, defsec, delfin454000, exd0tpy, fatherOfBlocks, hansfriese, ignacio, joestakey, kenta, ladboy233, m_Rassska, mics, minhquanym, oyc_109, pashov, reassor, robee, s3cunda, sach1r0, saian, sashik_eth, scaraven, sikorico, simon135, slywaters
156.0497 USDC - $156.05
| Issue | Instances | |
|---|---|---|
| 1 | Using calldata instead of memory for read-only arguments in external functions saves gas | 4 |
| 2 | Avoid contract existence checks by using solidity version 0.8.10 or later | 58 |
| 3 | State variables should be cached in stack variables rather than re-reading them from storage | 4 |
| 4 | internal functions only called once can be inlined to save gas | 7 |
| 5 | Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement | 3 |
| 6 | Using > 0 costs more gas than != 0 when used on a uint in a require() statement | 3 |
| 7 | It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied | 2 |
| 8 | Splitting require() statements that use && saves gas | 6 |
| 9 | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 6 |
| 10 | Duplicated require()/revert() checks should be refactored to a modifier or function | 3 |
| 11 | require() or revert() statements that check input arguments should be at the top of the function | 3 |
| 12 | Superfluous event fields | 9 |
| 13 | Use custom errors rather than revert()/require() strings to save gas | 37 |
| 14 | Functions guaranteed to revert when called by normal users can be marked payable | 22 |
Total: 167 instances over 14 issues
calldata instead of memory for read-only arguments in external functions saves gasWhen a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution.
If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one
There are 4 instances of this issue:
File: src/contracts/LiquidityReserve.sol #1 37: string memory _tokenName,
File: src/contracts/LiquidityReserve.sol #2 38: string memory _tokenSymbol,
File: src/contracts/Yieldy.sol #3 30: string memory _tokenName,
File: src/contracts/Yieldy.sol #4 31: string memory _tokenSymbol,
Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (700 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value
There are 58 instances of this issue:
File: src/contracts/BatchRequests.sol /// @audit canBatchTransactions() 19: IStaking(contracts[i]).canBatchTransactions() /// @audit canBatchTransactions() 37: bool canBatch = IStaking(contracts[i]).canBatchTransactions(); /// @audit canBatchTransactions() 57: IStaking(contracts[_index]).canBatchTransactions()
File: src/contracts/LiquidityReserve.sol /// @audit balanceOf() 64: uint256 stakingTokenBalance = IERC20Upgradeable(stakingToken).balanceOf( /// @audit safeTransferFrom() 75: IERC20Upgradeable(stakingToken).safeTransferFrom( /// @audit approve() 81: IERC20Upgradeable(rewardToken).approve( /// @audit balanceOf() 106: uint256 stakingTokenBalance = IERC20Upgradeable(stakingToken).balanceOf( /// @audit balanceOf() 109: uint256 rewardTokenBalance = IERC20Upgradeable(rewardToken).balanceOf( /// @audit coolDownInfo() 113 uint256 coolDownAmount = IStaking(stakingContract) 114: .coolDownInfo(address(this)) /// @audit safeTransferFrom() 121: IERC20Upgradeable(stakingToken).safeTransferFrom( /// @audit balanceOf() 140: uint256 stakingTokenBalance = IERC20Upgradeable(stakingToken).balanceOf( /// @audit balanceOf() 143: uint256 rewardTokenBalance = IERC20Upgradeable(rewardToken).balanceOf( /// @audit coolDownInfo() 146 uint256 coolDownAmount = IStaking(stakingContract) 147: .coolDownInfo(address(this)) /// @audit balanceOf() 171: IERC20Upgradeable(stakingToken).balanceOf(address(this)) >= /// @audit safeTransfer() 177: IERC20Upgradeable(stakingToken).safeTransfer( /// @audit safeTransferFrom() 198: IERC20Upgradeable(rewardToken).safeTransferFrom( /// @audit safeTransfer() 204: IERC20Upgradeable(stakingToken).safeTransfer( /// @audit coolDownInfo() 216 uint256 coolDownAmount = IStaking(stakingContract) 217: .coolDownInfo(address(this)) /// @audit balanceOf() 220: uint256 amount = IERC20Upgradeable(rewardToken).balanceOf(
File: src/contracts/Migration.sol /// @audit REWARD_TOKEN() 28: OLD_YIELDY_TOKEN = IStakingV1(_oldContract).REWARD_TOKEN(); /// @audit STAKING_TOKEN() 29: address stakingToken = IStaking(_newContract).STAKING_TOKEN(); /// @audit approve() 31: IYieldy(OLD_YIELDY_TOKEN).approve(_oldContract, type(uint256).max); /// @audit approve() 32: IERC20Upgradeable(stakingToken).approve( /// @audit balanceOf() 44: uint256 userWalletBalance = IYieldy(OLD_YIELDY_TOKEN).balanceOf( /// @audit transferFrom() 48: IYieldy(OLD_YIELDY_TOKEN).transferFrom(
File: src/contracts/Staking.sol /// @audit approve() 79: IERC20(TOKE_POOL).approve(CURVE_POOL, type(uint256).max); /// @audit approve() 83: IERC20(STAKING_TOKEN).approve(TOKE_POOL, type(uint256).max); /// @audit approve() 84: IERC20Upgradeable(YIELDY_TOKEN).approve( /// @audit approve() 88: IERC20Upgradeable(YIELDY_TOKEN).approve( /// @audit approve() 92: IERC20Upgradeable(TOKE_TOKEN).approve(COW_RELAYER, type(uint256).max); /// @audit safeTransfer() 132: IERC20Upgradeable(TOKE_TOKEN).safeTransfer(FEE_ADDRESS, feeAmount); /// @audit balanceOf() 144: uint256 totalTokeAmount = IERC20Upgradeable(TOKE_TOKEN).balanceOf( /// @audit safeTransfer() 147: IERC20Upgradeable(TOKE_TOKEN).safeTransfer( /// @audit balanceOf() 321: uint256 balance = ITokePool(TOKE_POOL).balanceOf(address(this)); /// @audit balanceOf() 372: uint256 tokePoolBalance = ITokePool(tokePoolContract).balanceOf( /// @audit totalSupply() 412: uint256 yieldyTotalSupply = IYieldy(YIELDY_TOKEN).totalSupply(); /// @audit safeTransferFrom() 419: IERC20Upgradeable(STAKING_TOKEN).safeTransferFrom( /// @audit creditsForTokenBalance() 442: IYieldy(YIELDY_TOKEN).creditsForTokenBalance(_amount), /// @audit transfer() 471: IYieldy(YIELDY_TOKEN).transfer( /// @audit tokenBalanceForCredits() 473: IYieldy(YIELDY_TOKEN).tokenBalanceForCredits(info.credits) /// @audit tokenBalanceForCredits() 487 uint256 totalAmountIncludingRewards = IYieldy(YIELDY_TOKEN) 488: .tokenBalanceForCredits(info.credits); /// @audit safeTransfer() 498: IERC20Upgradeable(STAKING_TOKEN).safeTransfer( /// @audit balanceOf() 519: uint256 walletBalance = IERC20Upgradeable(YIELDY_TOKEN).balanceOf( /// @audit tokenBalanceForCredits() 522: uint256 warmUpBalance = IYieldy(YIELDY_TOKEN).tokenBalanceForCredits( /// @audit creditsForTokenBalance() 545: IYieldy(YIELDY_TOKEN).creditsForTokenBalance(_amount); /// @audit tokenBalanceForCredits() 546 uint256 remainingAmount = IYieldy(YIELDY_TOKEN) 547: .tokenBalanceForCredits(remainingCreditsAmount); /// @audit safeTransferFrom() 559: IERC20Upgradeable(YIELDY_TOKEN).safeTransferFrom( /// @audit balanceOf() 582: uint256 reserveBalance = IERC20Upgradeable(STAKING_TOKEN).balanceOf( /// @audit exchange() 620: ICurvePool(CURVE_POOL).exchange( /// @audit coins() 634: address address0 = ICurvePool(CURVE_POOL).coins(0); /// @audit coins() 635: address address1 = ICurvePool(CURVE_POOL).coins(1); /// @audit get_dy() 664: ICurvePool(CURVE_POOL).get_dy(curvePoolFrom, curvePoolTo, _amount); /// @audit creditsForTokenBalance() 690: IYieldy(YIELDY_TOKEN).creditsForTokenBalance(_amount), /// @audit totalSupply() 711: uint256 staked = IYieldy(YIELDY_TOKEN).totalSupply(); /// @audit balanceOf() 730: IERC20Upgradeable(STAKING_TOKEN).balanceOf(address(this)) + /// @audit safeTransferFrom() 747: IERC20Upgradeable(STAKING_TOKEN).safeTransferFrom( /// @audit balanceOf() 755 uint256 stakingTokenBalance = IERC20Upgradeable(STAKING_TOKEN) 756: .balanceOf(address(this)); /// @audit setPreSignature() 770: ICowSettlement(COW_SETTLEMENT).setPreSignature(orderUid, true);
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There are 4 instances of this issue:
File: src/contracts/BatchRequests.sol #1 /// @audit contracts on line 18 19: IStaking(contracts[i]).canBatchTransactions()
File: src/contracts/BatchRequests.sol #2 /// @audit contracts on line 19 21: IStaking(contracts[i]).sendWithdrawalRequests();
File: src/contracts/BatchRequests.sol #3 /// @audit contracts on line 37 38: batch[i] = Batch(contracts[i], canBatch);
File: src/contracts/BatchRequests.sol #4 /// @audit contracts on line 56 57: IStaking(contracts[_index]).canBatchTransactions()
internal functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 7 instances of this issue:
File: src/contracts/LiquidityReserve.sol 134 function _calculateReserveTokenValue(uint256 _amount) 135 internal 136 view 137: returns (uint256)
File: src/contracts/Staking.sol 129: function _sendAffiliateFee(uint256 _amount) internal { 274 function _isClaimWithdrawAvailable(address _recipient) 275 internal 276: returns (bool) 342: function _getTokemakBalance() internal view returns (uint256) { 727: function contractBalance() internal view returns (uint256) {
File: src/contracts/Yieldy.sol 69: function _setIndex(uint256 _index) internal { 110 function _storeRebase( 111 uint256 _previousCirculating, 112 uint256 _profit, 113: uint256 _epoch
unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statementrequire(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }
There are 3 instances of this issue:
File: src/contracts/LiquidityReserve.sol #1 /// @audit require() on line 94 196: uint256 amountMinusFee = _amount - ((_amount * fee) / BASIS_POINTS);
File: src/contracts/Yieldy.sol #2 /// @audit require() on line 190 192: creditBalances[msg.sender] = creditBalances[msg.sender] - creditAmount;
File: src/contracts/Yieldy.sol #3 /// @audit require() on line 210 212: uint256 newValue = _allowances[_from][msg.sender] - _value;
> 0 costs more gas than != 0 when used on a uint in a require() statementThis change saves 6 gas per instance. The optimization works until solidity version 0.8.13 where there is a regression in gas costs.
There are 3 instances of this issue:
File: src/contracts/Staking.sol #1 410: require(_amount > 0, "Must have valid amount");
File: src/contracts/Staking.sol #2 572: require(_amount > 0, "Invalid amount");
File: src/contracts/Staking.sol #3 604: require(_amount > 0, "Invalid amount");
constant/non-immutable variables to zero than to let the default of zero be appliedNot overwriting the default for stack variables saves 8 gas. Storage and memory variables have larger savings
There are 2 instances of this issue:
File: src/contracts/Staking.sol #1 636: int128 from = 0;
File: src/contracts/Staking.sol #2 637: int128 to = 0;
require() statements that use && saves gasSee this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper
There are 6 instances of this issue:
File: src/contracts/LiquidityReserve.sol 44 require( 45 _stakingToken != address(0) && _rewardToken != address(0), 46 "Invalid address" 47: );
File: src/contracts/Migration.sol 20 require( 21 _oldContract != address(0) && _newContract != address(0), 22 "Invalid address" 23: );
File: src/contracts/Staking.sol 54 require( 55 _stakingToken != address(0) && 56 _yieldyToken != address(0) && 57 _tokeToken != address(0) && 58 _tokePool != address(0) && 59 _tokeManager != address(0) && 60 _tokeReward != address(0) && 61 _liquidityReserve != address(0), 62 "Invalid address" 63: ); 574 require( 575 !isUnstakingPaused && !isInstantUnstakingPaused, 576 "Unstaking is paused" 577: ); 605 require( 606 CURVE_POOL != address(0) && 607 (curvePoolFrom == 1 || curvePoolTo == 1), 608 "Invalid Curve Pool" 609: ); 611 require( 612 !isUnstakingPaused && !isInstantUnstakingPaused, 613 "Unstaking is paused" 614: );
uints/ints smaller than 32 bytes (256 bits) incurs overheadWhen using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
There are 6 instances of this issue:
File: src/contracts/Staking.sol 36: event LogSetCurvePool(address indexed curvePool, int128 to, int128 from); 36: event LogSetCurvePool(address indexed curvePool, int128 to, int128 from); 113: uint8 _v, 636: int128 from = 0; 637: int128 to = 0;
File: src/contracts/Yieldy.sol 32: uint8 _decimal
require()/revert() checks should be refactored to a modifier or functionSaves deployment costs
There are 3 instances of this issue:
File: src/contracts/LiquidityReserve.sol #1 192: require(isReserveEnabled, "Not enabled yet");
File: src/contracts/Staking.sol #2 604: require(_amount > 0, "Invalid amount");
File: src/contracts/Staking.sol #3 611 require( 612 !isUnstakingPaused && !isInstantUnstakingPaused, 613 "Unstaking is paused" 614: );
require() or revert() statements that check input arguments should be at the top of the functionChecks that involve constants should come before checks that involve state variables
There are 3 instances of this issue:
File: src/contracts/LiquidityReserve.sol #1 62: require(_stakingContract != address(0), "Invalid address");
File: src/contracts/Staking.sol #2 410: require(_amount > 0, "Must have valid amount");
File: src/contracts/Yieldy.sol #3 59: require(_stakingContract != address(0), "Invalid address");
block.timestamp and block.number are added to event information by default so adding them manually wastes gas
There are 9 instances of this issue:
File: src/contracts/Staking.sol 21: event LogSetEpochDuration(uint256 indexed blockNumber, uint256 duration); 22: event LogSetWarmUpPeriod(uint256 indexed blockNumber, uint256 period); 23: event LogSetCoolDownPeriod(uint256 indexed blockNumber, uint256 period); 24: event LogSetPauseStaking(uint256 indexed blockNumber, bool shouldPause); 25: event LogSetPauseUnstaking(uint256 indexed blockNumber, bool shouldPause); 27: uint256 indexed blockNumber, 31: uint256 indexed blockNumber, 34: event LogSetAffiliateFee(uint256 indexed blockNumber, uint256 fee);
File: src/contracts/Yieldy.sol 17: uint256 timestamp,
revert()/require() strings to save gasCustom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas
There are 37 instances of this issue:
File: src/contracts/LiquidityReserve.sol 25: require(msg.sender == stakingContract, "Not staking contract"); 44 require( 45 _stakingToken != address(0) && _rewardToken != address(0), 46 "Invalid address" 47: ); 61: require(!isReserveEnabled, "Already enabled"); 62: require(_stakingContract != address(0), "Invalid address"); 68 require( 69 stakingTokenBalance >= MINIMUM_LIQUIDITY, 70 "Not enough staking tokens" 71: ); 94: require(_fee <= BASIS_POINTS, "Out of range"); 105: require(isReserveEnabled, "Not enabled yet"); 163: require(_amount <= balanceOf(msg.sender), "Not enough lr tokens"); 170 require( 171 IERC20Upgradeable(stakingToken).balanceOf(address(this)) >= 172 amountToWithdraw, 173 "Not enough funds" 174: ); 192: require(isReserveEnabled, "Not enabled yet"); 215: require(isReserveEnabled, "Not enabled yet");
File: src/contracts/Migration.sol 20 require( 21 _oldContract != address(0) && _newContract != address(0), 22 "Invalid address" 23: );
File: src/contracts/Staking.sol 54 require( 55 _stakingToken != address(0) && 56 _yieldyToken != address(0) && 57 _tokeToken != address(0) && 58 _tokePool != address(0) && 59 _tokeManager != address(0) && 60 _tokeReward != address(0) && 61 _liquidityReserve != address(0), 62 "Invalid address" 63: ); 118: require(_recipient.amount > 0, "Must enter valid amount"); 143: require(_claimAddress != address(0), "Invalid address"); 408: require(!isStakingPaused, "Staking is paused"); 410: require(_amount > 0, "Must have valid amount"); 527 require( 528 _amount <= walletBalance + warmUpBalance, 529 "Insufficient Balance" 530: ); 572: require(_amount > 0, "Invalid amount"); 574 require( 575 !isUnstakingPaused && !isInstantUnstakingPaused, 576 "Unstaking is paused" 577: ); 586: require(reserveBalance >= _amount, "Not enough funds in reserve"); 604: require(_amount > 0, "Invalid amount"); 605 require( 606 CURVE_POOL != address(0) && 607 (curvePoolFrom == 1 || curvePoolTo == 1), 608 "Invalid Curve Pool" 609: ); 611 require( 612 !isUnstakingPaused && !isInstantUnstakingPaused, 613 "Unstaking is paused" 614: ); 644: require(from == 1 || to == 1, "Invalid Curve Pool"); 676: require(!isUnstakingPaused, "Unstaking is paused");
File: src/contracts/Yieldy.sol 58: require(stakingContract == address(0), "Already Initialized"); 59: require(_stakingContract != address(0), "Invalid address"); 83: require(_totalSupply > 0, "Can't rebase if not circulating"); 96: require(rebasingCreditsPerToken > 0, "Invalid change in supply"); 187: require(_to != address(0), "Invalid address"); 190: require(creditAmount <= creditBalances[msg.sender], "Not enough funds"); 210: require(_allowances[_from][msg.sender] >= _value, "Allowance too low"); 249: require(_address != address(0), "Mint to the zero address"); 257: require(_totalSupply < MAX_SUPPLY, "Max supply"); 279: require(_address != address(0), "Burn from the zero address"); 286: require(currentCredits >= creditAmount, "Not enough balance");
payableIf a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
There are 22 instances of this issue:
File: src/contracts/BatchRequests.sol 81: function addAddress(address _address) external onlyOwner { 89: function removeAddress(address _address) external onlyOwner {
File: src/contracts/LiquidityReserve.sol 57 function enableLiquidityReserve(address _stakingContract) 58 external 59: onlyOwner 92: function setFee(uint256 _fee) external onlyOwner { 188 function instantUnstake(uint256 _amount, address _recipient) 189 external 190: onlyStakingContract
File: src/contracts/Staking.sol 141: function transferToke(address _claimAddress) external onlyOwner { 157: function setCurvePool(address _curvePool) external onlyOwner { 167: function setAffiliateFee(uint256 _affiliateFee) external onlyOwner { 177: function setAffiliateAddress(address _affiliateAddress) external onlyOwner { 187: function shouldPauseStaking(bool _shouldPause) public onlyOwner { 197: function shouldPauseUnstaking(bool _shouldPause) external onlyOwner { 207: function shouldPauseInstantUnstaking(bool _shouldPause) external onlyOwner { 217: function setEpochDuration(uint256 duration) external onlyOwner { 226: function setWarmUpPeriod(uint256 _vestingPeriod) external onlyOwner { 235: function setCoolDownPeriod(uint256 _vestingPeriod) external onlyOwner { 246 function setTimeLeftToRequestWithdrawal(uint256 _timestamp) 247 external 248: onlyOwner 370: function unstakeAllFromTokemak() public onlyOwner { 769: function preSign(bytes calldata orderUid) external onlyOwner {
File: src/contracts/Yieldy.sol 54 function initializeStakingContract(address _stakingContract) 55 external 56: onlyRole(ADMIN_ROLE) 78 function rebase(uint256 _profit, uint256 _epoch) 79 external 80: onlyRole(REBASE_ROLE) 236 function mint(address _address, uint256 _amount) 237 external 238: onlyRole(MINTER_BURNER_ROLE) 266 function burn(address _address, uint256 _amount) 267 external 268: onlyRole(MINTER_BURNER_ROLE)
#0 - moose-code
2022-07-15T13:40:12Z
Power