Platform: Code4rena
Start Date: 16/12/2021
Pot Size: $100,000 USDC
Total HM: 21
Participants: 25
Period: 7 days
Judge: alcueca
Total Solo HM: 12
Id: 66
League: ETH
Rank: 12/25
Findings: 3
Award: $1,392.90
🌟 Selected for report: 11
🚀 Solo Findings: 0
717.2479 USDC - $717.25
Jujic
There is no upper limit on poolColl.tokens[], it increments each time when a new collateral is added. Eventually, as the count of collateral increases, gas cost of smart contract calls will raise and that there is no implemented function to reduce the array size.
For every call getVC() function which computed contain the VC value of a given collateralAddress is listed in poolColl.tokens[] array, the gas consumption can be more expensive each time that a new collateral address is appended to the array, until reaching an "Out of Gas" error or a "Block Gas Limit" in the worst scenario.
Remix
Array's length should be checked.
#0 - kingyetifinance
2022-01-05T08:02:13Z
@LilYeti: This is a known problem, and we are yet to test the upper limits of the contracts as is. Not sure how more theoretical issues like these are scored, but I would agree with that it is a medium to high risk based on how likely it is to happen * the potential effects. The worst possible outcome is that funds are locked in the protocol because it costs too much gas to do a withdrawal. We are still doing analysis on this, judges do what you want with this information.
#1 - kingyetifinance
2022-01-05T10:15:09Z
We would actually recommend it be a severity level 2, but it does have high potential risk.
Jujic
Here you could use ++i to save gas since it is more efficient then i++.
The same situation are in other scope contracts where loops use.
#0 - kingyetifinance
2022-01-03T06:39:30Z
@LilYeti : Duplicate with issue #12
Jujic
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met.
require(_ratio < 1100000000000000000, "ratio must be less than 1.10 => greater than 1.1 would mean taking out more YUSD than collateral VC");
Shorten the revert strings to fit in 32 bytes.
#0 - kingyetifinance
2022-01-03T06:40:40Z
@LilYeti: Related to issue #3 but saying shorten strings instead of for existing ones.
#1 - 0xtruco
2022-01-14T05:41:12Z
Resolved this and #266 #249 #176 #161 #90 in https://github.com/code-423n4/2021-12-yetifinance/pull/28
🌟 Selected for report: Jujic
97.5905 USDC - $97.59
Jujic
IERC20 public immutable JLP; IERC20 public immutable JOE;
REmix
🌟 Selected for report: Jujic
97.5905 USDC - $97.59
Jujic
Some of the variable can be cached to slightly reduce gas usage
dollarCap // can be cashed decayTime // can be cashed
Remix
🌟 Selected for report: Jujic
Jujic
user.amount JOE
Caching the user.amount and JOE can save gas.
🌟 Selected for report: Jujic
97.5905 USDC - $97.59
Jujic
contract WJLP does not need to import this contract in production
import "hardhat/console.sol";
Remix
Remove this contract to reduce the size of the contract and thus save some deployment gas.
#0 - 0xtruco
2022-01-14T07:16:45Z
Resolved
17.7859 USDC - $17.79
Jujic
The latest version of solc compiler is 8.6. Most contracts allow use of solc version >=0.6.0 <0.7.0, which is fairly dated. This may be a carry-over from previous versions of project to minimize porting code to handle breaking changes across solc 0.7.0 or 0.8.0.
Upgrading the solc compiler to 0.8 will give the latest compiler benefits including bug fixes, security enhancements and overall optimizations especially the in-built overflow/underflow checks which may give gas savings by avoiding expensive SafeMath.
Remix
Consider porting over code to solc >= 0.8.0 for bug fixes, security enhancements and overall optimizations for gas savings.
#0 - kingyetifinance
2022-01-06T07:39:05Z
@LilYeti: This is specifically saying it is for gas while #21 and others say for security. Acknowledged but won't be fixed. For instance not making calls to safemath may safe gas.
43.9157 USDC - $43.92
Jujic
Removing the assignment will save gas.
_totalSupply = 0;
Remove the assignment.
#0 - kingyetifinance
2022-01-06T07:43:00Z
@LilYeti: Yes, but this will save a miniscule amount of gas on deployment. Not much to be saved here. / not used by users.
#1 - alcueca
2022-01-15T06:25:29Z
I'll leave it as a gas improvement, instead of upgrading it to a code style issue
🌟 Selected for report: Jujic
97.5905 USDC - $97.59
Jujic
deploymentTime = block.timestamp; uint public constant BOOTSTRAP_PERIOD = 14 days; deploymentTime.add(BOOTSTRAP_PERIOD) // doesn't overflow
Remix
I recommend not use Safemath for this operation.
43.9157 USDC - $43.92
Jujic
Each function part of contract's external interface is part of the function dispatch, i.e., every time a contract is called, it goes through a switch statement (a set of eq ... JUMPI blocks in EVM) matching the selector of each externally available functions with the chosen function selector (the first 4 bytes of calldata). This means that any unnecessary function that is part of contract's external interface will lead to more gas for (almost) every single function calls to the contract. There are several cases where constants were made public. This is unnecessary; the constants can simply be readfrom the verified contract, i.e., it is unnecessary to expose it with a public function.
Example:
uint256 public constant BOOTSTRAP_PERIOD = 14 days;
Remix
#0 - 0xtruco
2022-01-14T06:15:44Z
Fix this and #271 in https://github.com/code-423n4/2021-12-yetifinance/pull/29
43.9157 USDC - $43.92
Jujic
The uint percentBacked can not be negative.
uint256 percentBacked = _collateralVCBalance.mul(1e18).div(_totalVCBalance); require(percentBacked <= 1e18 && percentBacked >= 0, "percent backed out of bounds");
Remix
You should remove the percentBacked >= 0 from require to save some gas.
Jujic
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.
function getVC() external view override returns (uint totalVC) { for (uint i = 0; i < poolColl.tokens.length; i++) { address collateral = poolColl.tokens[i]; uint amount = poolColl.amounts[i]; uint collateralVC = whitelist.getValueVC(collateral, amount); totalVC = totalVC.add(collateralVC); } return totalVC; }
Remix
Caching len = poolColl.tokens.length and using the len instead will save gas.
#0 - kingyetifinance
2022-01-03T06:42:11Z
Duplicate with #14
#1 - alcueca
2022-01-15T07:17:49Z
Duplicate #283
🌟 Selected for report: defsec
Also found by: 0x1f8b, Jujic, WatchPug, broccolirob, certora, cmichel, csanuragjain, hyh, jayjonah8, kenzo, robee, sirhashalot
Jujic
The IERC20(_collateral).transfer(_to, _amount) function return a boolean value indicating success. This parameter needs to be checked for success.
Some tokens do not revert if the transfer failed but return false instead.
The _sendCollateral function does not check the return value of this function.
Tokens that don't actually perform the transfer and return false are still counted as a correct transfer. Furthermore, tokens that do not correctly implement the EIP20 standard, like USDT which does not return a success boolean, will revert. This will affect on the work of the `sendCollaterals()' which must be called by borrower operations, trove manager, or stability pool in protocol.
bool sent = IERC20(_collateral).transfer(_to, _amount); require(sent);
Remix
We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.
#0 - kingyetifinance
2022-01-05T06:12:17Z
@LilYeti: Duplicate with issue #1 safe transfer error
#1 - kingyetifinance
2022-01-10T06:07:54Z
Fixed, nothing new compared to https://github.com/code-423n4/2021-12-yetifinance-findings/issues/94
#2 - alcueca
2022-01-15T07:32:33Z
Duplicate of #94