Platform: Code4rena
Start Date: 08/05/2023
Pot Size: $90,500 USDC
Total HM: 17
Participants: 102
Period: 7 days
Judge: 0xean
Total Solo HM: 4
Id: 236
League: ETH
Rank: 30/102
Findings: 2
Award: $688.35
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: brgltd
Also found by: 0x73696d616f, 0xAce, 0xSmartContract, 0xWaitress, 0xkazim, 0xnev, Aymen0909, BGSecurity, Bauchibred, Cayo, ChrisTina, Franfran, IceBear, Infect3d, Kose, Lilyjjo, PNS, RaymondFam, Sathish9098, Team_Rocket, Udsen, YakuzaKiawe, YoungWolves, berlin-101, bin2chen, btk, codeslide, fatherOfBlocks, frazerch, kodyvim, koxuan, lfzkoala, lukris02, matrix_0wl, nadin, naman1778, sashik_eth, tnevler, volodya, wonjun, yjrwkk
56.6347 USDC - $56.63
| Issue Count | Issues | Instances |
|---|---|---|
| [L-1] | Converting a string to bytes using bytes(string),Running out of gas can occur if the string is excessively large | 1 |
| [L-2] | LACK OF CHECKS THE INTEGER RANGES | 6 |
| [L-3] | Missing Event for initialize | 5 |
| [L-4] | Upgrade OpenZeppelin Contract/contracts-upgradeable Dependency | 2 |
| [L-5] | Even with the onlyOwner modifier, it is best practice to use the re-entrancy pattern | 15 |
| [L-6] | INITIALIZE() FUNCTION CAN BE CALLED BY ANYBODY (Initializer could be front-run) | 6 |
| [L-7] | Use safeTransferOwnership instead of transferOwnership function | 2 |
| [L-8] | DECIMALS() NOT PART OF ERC20 STANDARD | 2 |
| [L-9] | Missing ReEntrancy Guard to external functions when transfer tokens | 2 |
| [L-10] | address() function is only available in Solidity code | 8 |
| [L-11] | Insufficient coverage | - |
| [L-12] | Project Upgrade and Stop Scenario should be | - |
| Issue Count | Issues | Instances |
|---|---|---|
| [NC-1] | Add a timelock to critical functions | 15 |
| [NC-2] | No SAME VALUE INPUT CONTROL | 15 |
| [NC-3] | Critical changes should use two-step procedure | 15 |
| [NC-4] | For functions, follow Solidity standard naming conventions (internal function style rule) | 4 |
| [NC-5] | Use a more recent version of solidity | - |
| [NC-6] | Contract layout and order of functions | - |
| [NC-7] | Tokens accidentally sent to the contract cannot be recovered | - |
| [NC-8] | Use SMTChecker | - |
| [NC-9] | According to the syntax rules, use => mapping ( instead of => mapping( using spaces as keyword | - |
| [NC-10] | Reduce the inheritance list | 5 |
| [NC-11] | Need Fuzz testing for unchecked | 2 |
If the gas required for the string-to-bytes conversion exceeds the gas limit set for a particular transaction or block, the transaction will fail due to an out-of-gas exception. The exact gas limit can vary based on the network and configuration
FILE: Breadcrumbs2023-05-venus/contracts/Pool/PoolRegistry.sol 440: require(bytes(name).length <= 100, "Pool's name is too large");
String lengths to ensure it stays within acceptable gas limits and to find the optimal chunk size for your specific use case
Consider edge cases, and conduct security audits to identify potential vulnerabilities or issues related to integer ranges. Taking a proactive approach to ensure the correctness and safety of your code is essential for building secure smart contracts
FILE: Breadcrumbs2023-05-venus/contracts/Comptroller.sol loopLimit is not checked before set _setMaxLoopsLimit. loopLimit can be zero value. Should be checked before set _setMaxLoopsLimit function initialize(uint256 loopLimit, address accessControlManager) external initializer { __Ownable2Step_init(); __AccessControlled_init_unchained(accessControlManager); _setMaxLoopsLimit(loopLimit); }
FILE: Breadcrumbs2023-05-venus/contracts/Rewards/RewardsDistributor.sol loopLimit_ is not checked before set _setMaxLoopsLimit. loopLimit_ can be zero value. Should be checked before set _setMaxLoopsLimit function initialize( Comptroller comptroller_, IERC20Upgradeable rewardToken_, uint256 loopsLimit_, address accessControlManager_ ) external initializer { comptroller = comptroller_; rewardToken = rewardToken_; __Ownable2Step_init(); __AccessControlled_init_unchained(accessControlManager_); _setMaxLoopsLimit(loopsLimit_); }
Require(loopLimit > 0, "the loopLimit can't be zero value ");
Events help non-contract tools to track changes, and events prevent users from being surprised by changes Issuing event-emit during initialization is a detail that many projects skip
Add Event-Emit
An outdated OZ version 4.8.0 is used (which has known vulnerabilities, see: https://security.snyk.io/package/npm/@openzeppelin%2Fcontracts/4.8.0)
FILE: package.json 51: "@openzeppelin/contracts": "^4.8.0", 52: "@openzeppelin/contracts-upgradeable": "^4.8.0",
Update OpenZeppelin Contracts Usage in package.json and require at least version of 4.8.3 via >=4.8.3
It's still good practice to apply the reentry model as a precautionary measure in case the code is changed in the future to remove the onlyOwner modifier or the contract is used as a base contract for other contracts.
Using the reentry modifier provides an additional layer of security and ensures that your code is protected from potential reentry attacks regardless of any other security measures you take.
So even if you followed the "check-effects-interactions" pattern correctly, it's still considered good practice to use the reentry modifier
FILE: Breadcrumbs2023-05-venus/contracts/Comptroller.sol 927: function addRewardsDistributor(RewardsDistributor _rewardsDistributor) external onlyOwner { 961: function setPriceOracle(PriceOracle newOracle) external onlyOwner { 973: function setMaxLoopsLimit(uint256 limit) external onlyOwner {
FILE: 2023-05-venus/contracts/VToken.sol 505: function setProtocolShareReserve(address payable protocolShareReserve_) external onlyOwner { 515: function setShortfallContract(address shortfall_) external onlyOwner {
FILE: 2023-05-venus/contracts/Shortfall/Shortfall.sol 348: function updatePoolRegistry(address _poolRegistry) external onlyOwner {
FILE: 2023-05-venus/contracts/Rewards/RewardsDistributor.sol 181: function grantRewardToken(address recipient, uint256 amount) external onlyOwner { 219: function setContributorRewardTokenSpeed(address contributor, uint256 rewardTokenSpeed) external onlyOwner { 249: function setMaxLoopsLimit(uint256 limit) external onlyOwner {
FILE: Breadcrumbs2023-05-venus/contracts/Pool/PoolRegistry.sol 188: function setProtocolShareReserve(address payable protocolShareReserve_) external onlyOwner { 198: function setShortfallContract(Shortfall shortfall_) external onlyOwner {
FILE: 2023-05-venus/contracts/RiskFund/RiskFund.sol 99: function setPoolRegistry(address _poolRegistry) external onlyOwner { 110: function setShortfallContractAddress(address shortfallContractAddress_) external onlyOwner { 126: function setPancakeSwapRouter(address pancakeSwapRouter_) external onlyOwner { 205: function setMaxLoopsLimit(uint256 limit) external onlyOwner {
modifier noReentrant() { require(!locked, "Reentrant call"); locked = true; _; locked = false; }
initialize() function can be called anybody when the contract is not initialized.
More importantly, if someone else runs this function, they will have full authority because of the __Ownable2Step_init() function.
FILE: 2023-05-venus/contracts/Comptroller.sol 138: function initialize(uint256 loopLimit, address accessControlManager) external initializer {
FILE: 2023-05-venus/contracts/VToken.sol 58: function initialize( address underlying_, ComptrollerInterface comptroller_, InterestRateModel interestRateModel_, uint256 initialExchangeRateMantissa_, string memory name_, string memory symbol_, uint8 decimals_, address admin_, address accessControlManager_, RiskManagementInit memory riskManagement, uint256 reserveFactorMantissa_ ) external initializer {
FILE: 2023-05-venus/contracts/Shortfall/Shortfall.sol 131: function initialize( address convertibleBaseAsset_, IRiskFund riskFund_, uint256 minimumPoolBadDebt_, address accessControlManager_ ) external initializer {
FILE: 2023-05-venus/contracts/Rewards/RewardsDistributor.sol 111: function initialize( Comptroller comptroller_, IERC20Upgradeable rewardToken_, uint256 loopsLimit_, address accessControlManager_ ) external initializer {
FILE: Breadcrumbs2023-05-venus/contracts/Pool/PoolRegistry.sol 164: function initialize( VTokenProxyFactory vTokenFactory_, JumpRateModelFactory jumpRateFactory_, WhitePaperInterestRateModelFactory whitePaperFactory_, Shortfall shortfall_, address payable protocolShareReserve_, address accessControlManager_ ) external initializer {
FILE: Breadcrumbs2023-05-venus/contracts/RiskFund/RiskFund.sol 73: function initialize( address pancakeSwapRouter_, uint256 minAmountToConvert_, address convertibleBaseAsset_, address accessControlManager_, uint256 loopsLimit_ ) external initializer {
Add a control that makes initialize() only call the Deployer Contract;
if (msg.sender != DEPLOYER_ADDRESS) { revert NotDeployer(); }
Use safeTransferOwnership which is safer. Use it as it is more secure due to 2-stage ownership transfer
FILE: Breadcrumbs2023-05-venus/contracts/VToken.sol 1395: _transferOwnership(admin_);
FILE: Breadcrumbs2023-05-venus/contracts/Pool/PoolRegistry.sol 245: comptrollerProxy.transferOwnership(msg.sender);
decimals() is not part of the official ERC20 standard and might fail for tokens that do not implement it. While in practice it is very unlikely, as usually most of the tokens implement it, this should still be considered as a potential issue.
FILE: Breadcrumbs2023-05-venus/contracts/Pool/PoolRegistry.sol 284: uint256 underlyingDecimals = IERC20Metadata(input.asset).decimals();
FILE: 2023-05-venus/contracts/Lens/PoolLens.sol 361: underlyingDecimals = IERC20Metadata(vToken.underlying()).decimals();
If there are external calls, particularly involving token transfers or interactions with other contracts, there may be a potential for reentrancy vulnerabilities. Even with check-effect-interaction pattern the ReEntrancyGuard gives extra layer of security.
claimRewardToken function transfer the tokens using _grantRewardToken internal functions.
FILE: Breadcrumbs2023-05-venus/contracts/Rewards/RewardsDistributor.sol 277: function claimRewardToken(address holder, VToken[] memory vTokens) public { uint256 vTokensCount = vTokens.length; _ensureMaxLoops(vTokensCount); for (uint256 i; i < vTokensCount; ++i) { VToken vToken = vTokens[i]; require(comptroller.isMarketListed(vToken), "market must be listed"); Exp memory borrowIndex = Exp({ mantissa: vToken.borrowIndex() }); _updateRewardTokenBorrowIndex(address(vToken), borrowIndex); _distributeBorrowerRewardToken(address(vToken), holder, borrowIndex); _updateRewardTokenSupplyIndex(address(vToken)); _distributeSupplierRewardToken(address(vToken), holder); } rewardTokenAccrued[holder] = _grantRewardToken(holder, rewardTokenAccrued[holder]); //@audit Token transfer call }
Use Openzeppelin Re-Entrancy pattern.
Address() function cannot be used outside the context of a contract. If you are interacting with the contract externally, such as through a web3 library or a command-line interface this address() function method is not compatible.
FILE: Breadcrumbs2023-05-venus/contracts/Pool/PoolRegistry.sol 286: _updateRewardTokenBorrowIndex(address(vToken), borrowIndex); 287: _distributeBorrowerRewardToken(address(vToken), holder, borrowIndex); 288: _updateRewardTokenSupplyIndex(address(vToken)); 289: _distributeSupplierRewardToken(address(vToken), holder); 315: _updateRewardTokenSupplyIndex(address(vToken)); 311: if (rewardTokenSupplySpeeds[address(vToken)] != supplySpeed) { 318: rewardTokenSupplySpeeds[address(vToken)] = supplySpeed; 330: rewardTokenBorrowSpeeds[address(vToken)] = borrowSpeed;
Recommended Mitigation:
Need to use the appropriate methods or functions provided by those tools to obtain the contract address
Due to its capacity, test coverage is expected to be 100%.
What is the overall line coverage percentage provided by your tests?: 94.21
At the start of the project, the system may need to be stopped or upgraded, I suggest you have a script beforehand and add it to the documentation. This can also be called an ” EMERGENCY STOP (CIRCUIT BREAKER) PATTERN “.
https://github.com/maxwoe/solidity_patterns/blob/master/security/EmergencyStop.sol
It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user). Consider adding a timelock to
FILE: Breadcrumbs2023-05-venus/contracts/Comptroller.sol 927: function addRewardsDistributor(RewardsDistributor _rewardsDistributor) external onlyOwner { 961: function setPriceOracle(PriceOracle newOracle) external onlyOwner { 973: function setMaxLoopsLimit(uint256 limit) external onlyOwner {
FILE: 2023-05-venus/contracts/VToken.sol 505: function setProtocolShareReserve(address payable protocolShareReserve_) external onlyOwner { 515: function setShortfallContract(address shortfall_) external onlyOwner {
FILE: 2023-05-venus/contracts/Shortfall/Shortfall.sol 348: function updatePoolRegistry(address _poolRegistry) external onlyOwner {
FILE: 2023-05-venus/contracts/Rewards/RewardsDistributor.sol 181: function grantRewardToken(address recipient, uint256 amount) external onlyOwner { 219: function setContributorRewardTokenSpeed(address contributor, uint256 rewardTokenSpeed) external onlyOwner { 249: function setMaxLoopsLimit(uint256 limit) external onlyOwner {
FILE: Breadcrumbs2023-05-venus/contracts/Pool/PoolRegistry.sol 188: function setProtocolShareReserve(address payable protocolShareReserve_) external onlyOwner { 198: function setShortfallContract(Shortfall shortfall_) external onlyOwner {
FILE: 2023-05-venus/contracts/RiskFund/RiskFund.sol 99: function setPoolRegistry(address _poolRegistry) external onlyOwner { 110: function setShortfallContractAddress(address shortfallContractAddress_) external onlyOwner { 126: function setPancakeSwapRouter(address pancakeSwapRouter_) external onlyOwner { 205: function setMaxLoopsLimit(uint256 limit) external onlyOwner {
FILE: 2023-05-venus/contracts/Comptroller.sol function setPriceOracle(PriceOracle newOracle) external onlyOwner { require(address(newOracle) != address(0), "invalid price oracle address"); PriceOracle oldOracle = oracle; oracle = newOracle; emit NewPriceOracle(oldOracle, newOracle); } function setMaxLoopsLimit(uint256 limit) external onlyOwner { _setMaxLoopsLimit(limit); }
FILE: FILE: 2023-05-venus/contracts/VToken.sol 1407: function _setProtocolShareReserve(address payable protocolShareReserve_) internal { if (protocolShareReserve_ == address(0)) { revert ZeroAddressNotAllowed(); } address oldProtocolShareReserve = address(protocolShareReserve); protocolShareReserve = protocolShareReserve_; emit NewProtocolShareReserve(oldProtocolShareReserve, address(protocolShareReserve_)); } 1398: function _setShortfallContract(address shortfall_) internal { if (shortfall_ == address(0)) { revert ZeroAddressNotAllowed(); } address oldShortfall = shortfall; shortfall = shortfall_; emit NewShortfallContract(oldShortfall, shortfall_); }
FILE: 2023-05-venus/contracts/Shortfall/Shortfall.sol 348: function updatePoolRegistry(address _poolRegistry) external onlyOwner {
FILE: 2023-05-venus/contracts/Rewards/RewardsDistributor.sol 181: function grantRewardToken(address recipient, uint256 amount) external onlyOwner { 219: function setContributorRewardTokenSpeed(address contributor, uint256 rewardTokenSpeed) external onlyOwner { 249: function setMaxLoopsLimit(uint256 limit) external onlyOwner {
FILE: Breadcrumbs2023-05-venus/contracts/Pool/PoolRegistry.sol 188: function setProtocolShareReserve(address payable protocolShareReserve_) external onlyOwner { 198: function setShortfallContract(Shortfall shortfall_) external onlyOwner {
FILE: 2023-05-venus/contracts/RiskFund/RiskFund.sol 99: function setPoolRegistry(address _poolRegistry) external onlyOwner { 110: function setShortfallContractAddress(address shortfallContractAddress_) external onlyOwner { 126: function setPancakeSwapRouter(address pancakeSwapRouter_) external onlyOwner { 205: function setMaxLoopsLimit(uint256 limit) external onlyOwner {
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two-step procedure on the critical functions.
Consider adding a two-steps pattern on critical changes to avoid mistakenly transferring ownership of roles or critical functionalities to the wrong address
FILE: 2023-05-venus/contracts/Comptroller.sol function setPriceOracle(PriceOracle newOracle) external onlyOwner { require(address(newOracle) != address(0), "invalid price oracle address"); PriceOracle oldOracle = oracle; oracle = newOracle; emit NewPriceOracle(oldOracle, newOracle); } function setMaxLoopsLimit(uint256 limit) external onlyOwner { _setMaxLoopsLimit(limit); }
FILE: FILE: 2023-05-venus/contracts/VToken.sol 1407: function _setProtocolShareReserve(address payable protocolShareReserve_) internal { if (protocolShareReserve_ == address(0)) { revert ZeroAddressNotAllowed(); } address oldProtocolShareReserve = address(protocolShareReserve); protocolShareReserve = protocolShareReserve_; emit NewProtocolShareReserve(oldProtocolShareReserve, address(protocolShareReserve_)); } 1398: function _setShortfallContract(address shortfall_) internal { if (shortfall_ == address(0)) { revert ZeroAddressNotAllowed(); } address oldShortfall = shortfall; shortfall = shortfall_; emit NewShortfallContract(oldShortfall, shortfall_); }
FILE: 2023-05-venus/contracts/Shortfall/Shortfall.sol 348: function updatePoolRegistry(address _poolRegistry) external onlyOwner {
FILE: 2023-05-venus/contracts/Rewards/RewardsDistributor.sol 181: function grantRewardToken(address recipient, uint256 amount) external onlyOwner { 219: function setContributorRewardTokenSpeed(address contributor, uint256 rewardTokenSpeed) external onlyOwner { 249: function setMaxLoopsLimit(uint256 limit) external onlyOwner {
FILE: Breadcrumbs2023-05-venus/contracts/Pool/PoolRegistry.sol 188: function setProtocolShareReserve(address payable protocolShareReserve_) external onlyOwner { 198: function setShortfallContract(Shortfall shortfall_) external onlyOwner {
FILE: 2023-05-venus/contracts/RiskFund/RiskFund.sol 99: function setPoolRegistry(address _poolRegistry) external onlyOwner { 110: function setShortfallContractAddress(address shortfallContractAddress_) external onlyOwner { 126: function setPancakeSwapRouter(address pancakeSwapRouter_) external onlyOwner { 205: function setMaxLoopsLimit(uint256 limit) external onlyOwner {
Description The bellow codes don’t follow Solidity’s standard naming convention,
internal and private functions and variables : the mixedCase format starting with an underscore (_mixedCase starting with an underscore)
Latest solidity version is 0.8.19
CONTEXT ALL CONTRACT SCOPES
The Solidity style guide recommends declaring state variables before all functions.
(https://docs.soliditylang.org/en/v0.8.17/style-guide.html#order-of-layout)
CONTEXT ALL CONTRACT
Pragma statements
Import statements
Interfaces
Libraries
Contracts
Type declarations
State variables
Events
Modifiers
Functions
All contracts should follow the solidity style guide
It can’t be recovered if the tokens accidentally arrive at the contract address, which has happened to many popular projects, so I recommend adding a recovery code to your critical contracts.
Add this code:
/**
*/ function rescueERC20(address account, address externalToken, uint256 amount) public onlyOwner returns (bool) { IERC20(externalToken).transfer(account, amount); return true; } }
The highest tier of smart contract behavior assurance is formal mathematical verification. All assertions that are made are guaranteed to be true across all inputs → The quality of your asserts is the quality of your verification.
https://twitter.com/0xOwenThurm/status/1614359896350425088?t=dbG9gHFigBX85Rv29lOjIQ&s=19
FILE: 2023-05-venus/contracts/RiskFund/ReserveHelpers.sol 63: unchecked { balanceDifference = currentBalance - assetReserve; }
FILE: 2023-05-venus/contracts/BaseJumpRateModelV2.sol 184: unchecked { excessUtil = util - kink; }
#0 - c4-judge
2023-05-18T19:38:47Z
0xean marked the issue as grade-b
🌟 Selected for report: JCN
Also found by: 0xAce, 0xSmartContract, Aymen0909, K42, Rageur, Raihan, ReyAdmirado, SAAJ, SM3_SS, Sathish9098, Team_Rocket, Udsen, c3phas, codeslide, descharre, fatherOfBlocks, hunter_w3b, j4ld1na, lllu_23, matrix_0wl, naman1778, petrichor, pontifex, rapha, souilos, wahedtalash77
631.723 USDC - $631.72
| Issue Count | Issues | Instances | Gas Saved |
|---|---|---|---|
| [G-1] | Refactor the state variables to be packed into fewer storage slots | 3 | 60000 |
| [G-2] | USING CALLDATA INSTEAD OF MEMORY FOR READ-ONLY ARGUMENTS IN EXTERNAL FUNCTIONS SAVES GAS | 10 | 3000 |
| [G-3] | Refactor the "for" loop to save gas | 5 | Depends number of iterations |
| [G-4] | IF’s/require() statements that check input arguments should be at the top of the function | 1 | 2100 |
| [G-5] | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 4 | 120 |
| [G-6] | Caching msg.sender cause more gas | 3 | - |
| [G-7] | Avoid Emitting State Variables When Stack Variables Are Available | 2 | 244 |
| [G-8] | Use constants instead of type(uintx).max | 5 | - |
| [G-9] | Lack of input value checks cause a redeployment if any human/accidental errors | 9 | - |
| [G-10] | Use nested if and, avoid multiple check combinations | 10 | 90 |
| [G-11] | No need to evaluate all expressions to know if one of them is true | 4 | - |
| [G-12] | Amounts should be checked for 0 before calling a transfer | 7 | - |
| [G-13] | Use a more recent version of solidity | - | - |
| [G-14] | Non-usage of specific imports | - | - |
| [G-15] | Shorten the array rather than copying to a new one | 13 | - |
Instances (3)
Gas Saved : 60000 gas (3 Gsset )
If variables occupying the same slot are both written the same function or by the constructor, avoids a separate Gsset (20000 gas). Reads of the variables can also be cheaper.
Move _isComptroller bool variable bellow the oracle variable. So this will stored within single slot instead of 2 slots
FILE: 2023-05-venus/contracts/ComptrollerStorage.sol The lines shortened for better understanding 59: PriceOracle public oracle; + 115: bool internal constant _isComptroller = true; /** * @notice Multiplier used to calculate the maximum repayAmount when liquidating a borrow */ 64: uint256 public closeFactorMantissa; 101: mapping(address => bool) internal rewardsDistributorExists; - 115: bool internal constant _isComptroller = true;
/// @notice Time to wait for next bidder. initially waits for 10 blocks uint256 public nextBidderBlockLimit; /// @notice Time to wait for first bidder. initially waits for 100 blocks uint256 public waitForFirstBidder;
As per docs nextBidderBlockLimit,waitForFirstBidder these state variables only stores the waiting time for bidders.The waiting time not going to too high.
So its possible refactor uint256 to uint96. Can saves 2 slots and 40000 gas
In Solidity, the uint96 type represents an unsigned integer with a range from 0 to 2^96 - 1.The exact possible value to store in uint96 2^96 - 1 is 79,228,162,514,264,337,593,543,950,335.
79,228,162,514,264,337,593,543,950,335 seconds ≈ 2,513,031,118.1 years (assuming regular years)
Uint96 alone more than enough to store 2,513,031,118.1 years
FILE: Breadcrumbs2023-05-venus/contracts/Shortfall/Shortfall.sol 48: address public poolRegistry; + 63: uint96 public nextBidderBlockLimit; /// @notice Risk fund address 51: IRiskFund private riskFund; /// @notice Minimum USD debt in pool for shortfall to trigger 54: uint256 public minimumPoolBadDebt; /// @notice Incentive to auction participants, initial value set to 1000 or 10% 57: uint256 private incentiveBps; /// @notice Max basis points i.e., 100% 60: uint256 private constant MAX_BPS = 10000; /// @notice Time to wait for next bidder. initially waits for 10 blocks - 63: uint256 public nextBidderBlockLimit; /// @notice Time to wait for first bidder. initially waits for 100 blocks - 66: uint256 public waitForFirstBidder; + 66: uint96 public waitForFirstBidder; /// @notice base asset contract address 69: address public convertibleBaseAsset;
Instances (10)
Gas Saved : 3000 gas
calldata must be used when declaring an external function's dynamic parameters
When 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. Note that even if an interface defines a function as having memory arguments, it’s still valid for implementation contracs to use calldata arguments instead.
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
Note that I’ve also flagged instances where the function is public but can be marked as external since it’s not called by the contract, and cases where a constructor is involved
At least 300 gas saved for every instances
FILE: 2023-05-venus/contracts/Comptroller.sol + 154: function enterMarkets(address[] calldata vTokens) external override returns (uint256[] memory) { - 154: function enterMarkets(address[] memory vTokens) external override returns (uint256[] memory) {
FILE: Breadcrumbs2023-05-venus/contracts/VToken.sol 64: string memory name_, 65: string memory symbol_, 69: RiskManagementInit memory riskManagement,
FILE: 2023-05-venus/contracts/Rewards/RewardsDistributor.sol 166: Exp memory marketBorrowIndex 187: function updateRewardTokenBorrowIndex(address vToken, Exp memory marketBorrowIndex) external onlyComptroller { 198: VToken[] memory vTokens, 199: uint256[] memory supplySpeeds, 200: uint256[] memory borrowSpeeds
FILE: 2023-05-venus/contracts/Pool/PoolRegistry.sol 343: function updatePoolMetadata(address comptroller, VenusPoolMetaData memory _metadata) external {
As per Remix Test Reports Its possible to save 13 gas for every iterations
The saved gas increased as per iterations count. So can't find the approximate gas savings.
| GAS | TRANS COST | EXE COST |
|---|---|---|
| 26854 | 23351 | 1639 |
FILE: 2023-05-venus/contracts/Comptroller.sol 162: uint256[] memory results = new uint256[](len); 163: for (uint256 i; i < len; ++i) { 164: VToken vToken = VToken(vTokens[i]); 165: _addToMarket(vToken, msg.sender); 166: results[i] = NO_ERROR; 167: }
| GAS | TRANS COST | EXE COST |
|---|---|---|
| 26839 | 23338 | 1626 |
FILE: 2023-05-venus/contracts/Comptroller.sol 162: uint256[] memory results = new uint256[](len); 163: for (uint256 i; i < len; ++i) { - 164: VToken vToken = VToken(vTokens[i]); + 165: _addToMarket(VToken(vTokens[i]), msg.sender); - 165: _addToMarket(vToken, msg.sender); 166: results[i] = NO_ERROR; 167: }
vTokenSupply only used only once. So need to cache this with stack variable.
FILE: 2023-05-venus/contracts/Comptroller.sol - 263: uint256 vTokenSupply = VToken(vToken).totalSupply(); 264: Exp memory exchangeRate = Exp({ mantissa: VToken(vToken).exchangeRateStored() }); + 265: uint256 nextTotalSupply = mul_ScalarTruncateAddUInt(exchangeRate, VToken(vToken).totalSupply(), mintAmount); - 265: uint256 nextTotalSupply = mul_ScalarTruncateAddUInt(exchangeRate, vTokenSupply, mintAmount);
vToken only used once inside the function. Instead of caching can be used directly IERC20Upgradeable function call
FILE: 2023-05-venus/contracts/Shortfall/Shortfall.sol 223: for (uint256 i; i < marketsCount; ++i) { - 224: VToken vToken = VToken(address(auction.markets[i])); - 225: IERC20Upgradeable erc20 = IERC20Upgradeable(address(vToken.underlying())); + 225: IERC20Upgradeable erc20 = IERC20Upgradeable(address(VToken(address(auction.markets[i])).underlying()));
vToken No need to cache
FILE: 2023-05-venus/contracts/Shortfall/Shortfall.sol 374: for (uint256 i; i < marketsCount; ++i) { - 375: VToken vToken = auction.markets[i]; - 376: auction.marketDebt[vToken] = 0; + 376: auction.marketDebt[auction.markets[i]] = 0; 377: }
FILE: 2023-05-venus/contracts/Pool/PoolRegistry.sol 356: for (uint256 i = 1; i <= _numberOfPools; ++i) { - 357: address comptroller = _poolsByID[i]; - 358: _pools[i - 1] = (_poolByComptroller[comptroller]); + 358: _pools[i - 1] = (_poolByComptroller[_poolsByID[i]]); 359: }
Instances (1)
Gas Saved : 2100 gas
Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas) in a function that may ultimately revert in the unhappy case.
FILE: Breadcrumbs2023-05-venus/contracts/Shortfall/Shortfall.sol MAX_BPS constant check should come first + 163: require(bidBps <= MAX_BPS, "basis points cannot be more than 10000"); 161: require(_isStarted(auction), "no on-going auction"); 162: require(!_isStale(auction), "auction is stale, restart it"); - 163: require(bidBps <= MAX_BPS, "basis points cannot be more than 10000");
Instances(4)
Gas Saved : 120 gas
When 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 Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed
FILE: Breadcrumbs2023-05-venus/contracts/Pool/PoolRegistry.sol 29: uint224 public constant rewardTokenInitialIndex = 1e36; 128: uint32 blockNumber = safe32(getBlockNumber(), "block number exceeds 32 bits"); 433: uint32 blockNumber = safe32(getBlockNumber(), "block number exceeds 32 bits"); 461: uint32 blockNumber = safe32(getBlockNumber(), "block number exceeds 32 bits");
Use msg.sender directly without caching
FILE: 2023-05-venus/contracts/VToken.sol 136: address src = msg.sender; 628: address src = msg.sender; 648: address src = msg.sender;
Instances (2)
Gas Saved: 244 gas
In the instance below, we can emit the calldata value instead of emitting a storage value. This will result in using a cheap CALLDATALOAD instead of an expensive SLOAD
As per Remix Sample Test possible to save 122 gas for every instances
FILE: Breadcrumbs2023-05-venus/contracts/Comptroller.sol - 709: emit NewCloseFactor(oldCloseFactorMantissa, closeFactorMantissa); + 709: emit NewCloseFactor(oldCloseFactorMantissa, newCloseFactorMantissa);
FILE: 2023-05-venus/contracts/Rewards/RewardsDistributor.sol - 268: emit ContributorRewardsUpdated(contributor, rewardTokenAccrued[contributor]); + 268: emit ContributorRewardsUpdated(contributor, contributorAccrued);
type(uint256).max uses more gas in the distribution process and also for each transaction than constant usage
FILE: 2023-05-venus/contracts/Comptroller.sol 262: if (supplyCap != type(uint256).max) { 351: if (borrowCap != type(uint256).max) {
FILE: Breadcrumbs2023-05-venus/contracts/VToken.sol 1055: if (repayAmount == type(uint256).max) { 1314: startingAllowance = type(uint256).max; 1331: if (startingAllowance != type(uint256).max) {
Devoid of sanity/threshold/limit checks, critical parameters can be configured to invalid values, causing a variety of issues and breaking expected interactions within/between contracts. Consider adding proper uint256 validation. A worst case scenario would render the contract needing to be re-deployed in the event of human/accidental errors that involve value assignments to immutable variables.
If any human/accidental errors happen need to redeploy the contract so this create the huge gas lose
FILE: Breadcrumbs2023-05-venus/contracts/Comptroller.sol loopLimit is not checked before set _setMaxLoopsLimit. loopLimit can be zero value. Should be checked before set _setMaxLoopsLimit function initialize(uint256 loopLimit, address accessControlManager) external initializer { __Ownable2Step_init(); __AccessControlled_init_unchained(accessControlManager); _setMaxLoopsLimit(loopLimit); }
FILE: Breadcrumbs2023-05-venus/contracts/Rewards/RewardsDistributor.sol loopLimit_ is not checked before set _setMaxLoopsLimit. loopLimit_ can be zero value. Should be checked before set _setMaxLoopsLimit function initialize( Comptroller comptroller_, IERC20Upgradeable rewardToken_, uint256 loopsLimit_, address accessControlManager_ ) external initializer { comptroller = comptroller_; rewardToken = rewardToken_; __Ownable2Step_init(); __AccessControlled_init_unchained(accessControlManager_); _setMaxLoopsLimit(loopsLimit_); }
Instances(10)
Approximate Gas Saved: 90 gas
Using nested is cheaper than using && multiple check combinations. There are more advantages, such as easier to read code and better coverage reports.
As per Solidity reports possible to save 9 gas
FILE: Breadcrumbs2023-05-venus/contracts/Comptroller.sol 755: if (newCollateralFactorMantissa != 0 && oracle.getUnderlyingPrice(address(vToken)) == 0) { FILE: 2023-05-venus/contracts/VToken.sol 837: if (redeemTokens == 0 && redeemAmount > 0) { FILE: 2023-05-venus/contracts/Lens/PoolLens.sol 462: if (deltaBlocks > 0 && borrowSpeed > 0) { 483: if (deltaBlocks > 0 && supplySpeed > 0) { 506: if (borrowerIndex.mantissa == 0 && borrowIndex.mantissa > 0) { 526: if (supplierIndex.mantissa == 0 && supplyIndex.mantissa > 0) { FILE: 2023-05-venus/contracts/Rewards/RewardsDistributor.sol 348: if (supplierIndex == 0 && supplyIndex >= rewardTokenInitialIndex) { 386: if (borrowerIndex == 0 && borrowIndex >= rewardTokenInitialIndex) { 418: if (amount > 0 && amount <= rewardTokenRemaining) { 435: if (deltaBlocks > 0 && supplySpeed > 0) { 463: if (deltaBlocks > 0 && borrowSpeed > 0) {
Instances(4)
When we have a code expressionA || expressionB if expressionA is true then expressionB will not be evaluated and gas saved
FILE: 2023-05-venus/contracts/Comptroller.sol 449: if (skipLiquidityCheck || isDeprecated(VToken(vTokenBorrowed))) { FILE: 2023-05-venus/contracts/VToken.sol 805: require(redeemTokensIn == 0 || redeemAmountIn == 0, "one of redeemTokensIn or redeemAmountIn must be zero"); FILE: 2023-05-venus/contracts/Shortfall/Shortfall.sol 164: require( (auction.auctionType == AuctionType.LARGE_POOL_DEBT && ((auction.highestBidder != address(0) && bidBps > auction.highestBidBps) || (auction.highestBidder == address(0) && bidBps >= auction.startBidBps))) || (auction.auctionType == AuctionType.LARGE_RISK_FUND && ((auction.highestBidder != address(0) && bidBps < auction.highestBidBps) || (auction.highestBidder == address(0) && bidBps <= auction.startBidBps))), "your bid is not the highest" ); 364: require( (auction.startBlock == 0 && auction.status == AuctionStatus.NOT_STARTED) || auction.status == AuctionStatus.ENDED, "auction is on-going" );
Checking non-zero transfer values can avoid an expensive external call and save gas. While this is done at some places, it’s not consistently done in the solution. I suggest adding a non-zero-value check here
FILE: 2023-05-venus/contracts/Shortfall/Shortfall.sol 183: erc20.safeTransfer(auction.highestBidder, previousBidAmount); 187: erc20.safeTransferFrom(msg.sender, address(this), currentBidAmount); 229: erc20.safeTransfer(address(auction.markets[i]), bidAmount); 232: erc20.safeTransfer(address(auction.markets[i]), auction.marketDebt[auction.markets[i]]); 248: IERC20Upgradeable(convertibleBaseAsset).safeTransfer(auction.highestBidder, riskFundBidAmount);
FILE: Breadcrumbs2023-05-venus/contracts/Pool/PoolRegistry.sol 416: token.safeTransferFrom(from, address(this), amount);
FILE: 2023-05-venus/contracts/RiskFund/RiskFund.sol 194: IERC20Upgradeable(convertibleBaseAsset).safeTransfer(shortfall, amount);
Upgrade to the latest solidity version 0.8.19 to get additional gas savings. See latest release for reference: https://blog.soliditylang.org/2023/02/22/solidity-0.8.19-release-announcement/
CONTEXT: ALL SCOPE CONTRACTS
Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath
Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
In 0.8.15 the conditions necessary for inlining are relaxed. Benchmarks show that the change significantly decreases the bytecode size (which impacts the deployment cost) while the effect on the runtime gas usage is smaller.
In 0.8.17 prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call; Simplify the starting offset of zero-length operations to zero. More efficient overflow checks for multiplication.
in 0.8.19 prevents
CONTEXT: ALL SCOPE CONTRACTS
The current form of relative path import is not recommended for use because it can unpredictably pollute the namespace. Instead, the Solidity docs recommend specifying imported symbols explicitly. https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files
Inline-assembly can be used to shorten the array by changing the length slot, so that the entries don’t have to be copied to a new, shorter array
FILE: Breadcrumbs2023-05-venus/contracts/Lens/PoolLens.sol 126: VTokenBalances[] memory res = new VTokenBalances[](vTokenCount); 143: PoolData[] memory poolDataItems = new PoolData[](poolLength); 207: VTokenUnderlyingPrice[] memory res = new VTokenUnderlyingPrice[](vTokenCount); 228: RewardSummary[] memory rewardSummary = new RewardSummary[](rewardsDistributors.length); 256: BadDebt[] memory badDebts = new BadDebt[](markets.length); 390: VTokenMetadata[] memory res = new VTokenMetadata[](vTokenCount); 417: PendingReward[] memory pendingRewards = new PendingReward[](markets.length);
FILE: Breadcrumbs2023-05-venus/contracts/Shortfall/Shortfall.sol 219: uint256[] memory marketsDebt = new uint256[](marketsCount); 386: uint256[] memory marketsDebt = new uint256[](marketsCount);
FILE: Breadcrumbs2023-05-venus/contracts/Pool/PoolRegistry.sol 306: uint256[] memory newSupplyCaps = new uint256[](1); 307: uint256[] memory newBorrowCaps = new uint256[](1); 308: VToken[] memory vTokens = new VToken[](1); 355: VenusPool[] memory _pools = new VenusPool[](_numberOfPools);
#0 - c4-judge
2023-05-18T17:54:18Z
0xean marked the issue as grade-a
#1 - c4-sponsor
2023-05-23T21:48:10Z
chechu marked the issue as sponsor confirmed
#2 - chechu
2023-05-23T21:48:11Z
G-1 "1. Dispute Validity 2. Confirm" G-2 Confirm G-3 Confirm G-4 Confirm G-5 Confirm G-6 Confirm G-7 Confirm G-8 Dispute G-9 confirm G-10 confirm G-11 Acknowledge G-12 Dispute G-13 Acknowledge G-14 Acknowledge G-15 TBR