Platform: Code4rena
Start Date: 16/02/2023
Pot Size: $144,750 USDC
Total HM: 17
Participants: 154
Period: 19 days
Judge: Trust
Total Solo HM: 5
Id: 216
League: ETH
Rank: 33/154
Findings: 1
Award: $455.47
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: GalloDaSballo
Also found by: 0x3b, 0xAgro, 0xSmartContract, 0xTheC0der, 0xackermann, 0xnev, 0xsomeone, ABA, BRONZEDISC, Bjorn_bug, Bnke0x0, Breeje, Co0nan, CodeFoxInc, CodingNameKiki, DadeKuma, DeFiHackLabs, IceBear, Josiah, Kaysoft, Lavishq, MohammedRizwan, PaludoX0, PawelK, Phantasmagoria, Raiders, RaymondFam, Rickard, Rolezn, Sathish9098, SleepingBugs, SuperRayss, UdarTeam, Udsen, Viktor_Cortess, arialblack14, ast3ros, bin2chen, brgltd, btk, catellatech, ch0bu, chaduke, chrisdior4, codeislight, cryptonue, delfin454000, descharre, dontonka, emmac002, fs0c, hacker-dom, hansfriese, imare, lukris02, luxartvinsec, martin, matrix_0wl, peanuts, rbserver, shark, tnevler, trustindistrust, tsvetanovv, vagrant, yongskiws, zzzitron
455.4712 USDC - $455.47
The core contracts extended the functionality of the collaterals categories, with not only Ether can be added as collateral. And the protocol added a Vault function besides it to make the collateral assets’ efficiency better so that the stable coin’s backing assets generate interest as well.
A single Liquity code base can already be a good example of project to get audited. The Ethos protocol was built upon it and added several customized functions. These functionalities combined with each other makes the protocol more complex.
This audit contest partially does not include the Liquity protocol code base but a good understanding of it is needed. And Ethos Reserve expanded the functionality over it. As a result, the code base is quite large.
The overall quality of the code is good, inline comments provided are helpful in describing the function of the code. But in terms of documentations, there are improvements can be made, e.g., natspec documentation style is recommended in my opinion. Tests ran without issues and can be modified to run POCs easily.
The findings are made mainly focused on the best practice and code quality, documentations, etc. At the same time, there are some low-risk issues about the upgradeability and function arguments’ validations.
_treasuryAddress is not checkContractedIn the ActivePool contract, the _treasuryAddress is not checkContracted. I understand it can be an EOA, but as a big protocol which is going to generate millions of revenue and fees. I suggest put the checkContract function and make sure it is a multi-sig wallet.
+ checkContract(_treasuryAddress); - 　require(_treasuryAddress != address(0), "Treasury cannot be 0 address");
The code base includes quite a lot of renounceOwnership() function to make sure the protocol is decentralized enough to be permission-less and trusted.
At the same time, priceFeed contract is quite an important feature and it should not be controlled by a single entity. Price feed can be updated by the owner address in this the current situation.
Team should consider set it as guardianAddress or governanceAddress instead like what is done in the LUSDToken contract. Or you can make the transition into a community governable feature.
And same thing can also happen in CommunityIssuance contract.
token address and make sure it is a contract addressShould check the address to make sure it is a contract address or zero address.
Should import the checkContract function and use it as a checker for _token.
+ import "../Dependencies/CheckContract.sol"; ...... - 　contract ReaperVaultV2 is ReaperAccessControl, ERC20, IERC4626Events, AccessControlEnumerable, ReentrancyGuard { + contract ReaperVaultV2 is ReaperAccessControl, ERC20, IERC4626Events, AccessControlEnumerable, ReentrancyGuard, checkContract { ...... + checkContract(_token); 　　　　token = IERC20Metadata(_token);
tvlCap should be initialized as a reasonable valueThe tvlCap value is set to zero when the input of it is zero. This makes the Vault not usable at the beginning. When the tvlCap is zero usually it means there is no limit, so it is reasonable to set the value as unlimited value(type(uint256).max). If the dev wants to set the vault as unusable in the beginning it also can be done by setting the value extremely low, e.g. 1 wei. Anyways, if dev deploys the contract it is supposed to work in the first place.
Make a change as below:
token = IERC20Metadata(_token); constructionTime = block.timestamp; lastReport = block.timestamp; + if (_tvlCap == 0) { + tvlCap = type(uint256).max; // @audit the tvlCap should be set to a reasonable value. If the tvlCap is 0, the tvlCap will be set to the maximum value of uint256. + } else { tvlCap = _tvlCap; + } treasury = _treasury; lockedProfitDegradation = (DEGRADATION_COEFFICIENT * 46) / 10**6; // 6 hours in blocks
_collaterals and _strategies arrayDuring the initialization of the CollateralConfig contract, consider adding a length boundary for _collaterals array. Too many collaterals can make the loop cost too much gas and it may fail to fulfill its functions. The upper limit of the collateral’s length can be set as a number which is usually not likely to reach. Also should take the block’s gas limit into account.
Same thing can be mentioned about ReaperVaultV2 contract as below.
Also it is the same about the _withdrawalQueue in contract ReaperVaultV2.
If the queue is too long, it can block normal users’ withdraw in _withdraw function.
Recommendation examples: (also use cache to make it more gas efficient)
function initialize( address[] calldata _collaterals, uint256[] calldata _MCRs, uint256[] calldata _CCRs ) external override onlyOwner { 　require(!initialized, "Can only initialize once"); + uint256 length = _collaterals.length; - 　require(_collaterals.length != 0, "At least one collateral required"); + 　require(length != 0, "At least one collateral required"); + require(length <= 50, "too many kinds of collaterals");
　uint256 numStrategists = _strategists.length; + require(numStrategiests <= 10, "too many strategists") 　for (uint256 i = 0; i < numStrategists; i = i.uncheckedInc()) { 　 _grantRole(STRATEGIST, _strategists[i]); 　}
__gap into the contractWhen the contract is upgradeable, it is best practice to add a __gap into the last part of the contract for future upgrade.
As this mentioned, gap is an empty reserved space in the storage for contract’s future upgrading.
It can be tricky if gap is not in the inherited contract and you want to add more storages into the contract in the future upgrading version. Special cares need to be taken then.
So it is recommended that developers implement this and follow the best practice.
* caller must be returned. */ function _harvestCore(uint256 _debt) internal virtual returns (int256 roi, uint256 repayment); // @audit gap shbould be added here + /** + * @dev This empty reserved space is put in place to allow future versions to add new + * variables without shifting down storage in the inheritance chain. + * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps + */ + 　uint256[24] private __gap; // [24] should equal to [50 - (the number of how many used slots exist)] }
function updateVeloSwapPath( address _tokenIn, address _tokenOut, address[] calldata _path ) external virtual; // @audit gap shbould be added here + /** + * @dev This empty reserved space is put in place to allow future versions to add new + * variables without shifting down storage in the inheritance chain. + * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps + */ + 　uint256[49] private __gap; // [49] should equal to [50 - (the number of how many used slots exist)] }
If the implementation's initialize function is not called, there is such risk that any attacker can have a chance to selfdestruct the implementation contract to make the whole system break in the worst case scenario.
In the contract ReaperStrategyGranarySupplyOnly, the initialize function can be left uncalled. And even though developers have a plan to call it, it has a chance of being front-run.
I recommend follow the practice of OpenZeppelin’s documentation and call this function in the constructor to make sure no one can play with it.
Add this into the code base:
+ /// @custom:oz-upgrades-unsafe-allow constructor + constructor() { + _disableInitializers(); + }
This is a code style thing. I recommend follow the change as below.
// @audit the `_` is supposed to be used before 3 digits numbers, e.g. 2_000, but it is used with 2 digits numbers. - uint256 public yieldSplitTreasury = 20_00; // amount of yield to direct to treasury in BPS - uint256 public yieldSplitSP = 40_00; // amount of yield to direct to stability pool in BPS - uint256 public yieldSplitStaking = 40_00; // amount of yield to direct to OATH Stakers in BPS + uint256 public yieldSplitTreasury = 2_000; // amount of yield to direct to treasury in BPS + uint256 public yieldSplitSP = 4_000; // amount of yield to direct to stability pool in BPS + uint256 public yieldSplitStaking = 4_000; // amount of yield to direct to OATH Stakers in BPS
The best practice of solidity suggest error message should include contract’s name. This rule was adopted in some contract but it is not followed in some places. And I suggest fix them.
This is the good example showed in the BorrowerOperations contract by the sponsor.
But here the error message is inconsistent with the other part in the same contract.
And here the error message has no contract name at all.
Developers should follow the best practice and add the contract name into the error message to make improvement in terms of readability throughout the code base. I will give several examples in the code base here.
　　　　　　　if (_isRecoveryMode) { require(_maxFeePercentage <= DECIMAL_PRECISION, - "Max fee percentage must less than or equal to 100%"); + "BorrowerOperations: Max fee percentage must less than or equal to 100%"); } else { require(_maxFeePercentage >= BORROWING_FEE_FLOOR && _maxFeePercentage <= DECIMAL_PRECISION, - "Max fee percentage must be between 0.5% and 100%"); + "BorrowerOperations: Max fee percentage must be between 0.5% and 100%"); }
function _requireValidLUSDRepayment(uint _currentDebt, uint _debtRepayment) internal pure { - require(_debtRepayment <= _currentDebt.sub(LUSD_GAS_COMPENSATION), "BorrowerOps: Amount repaid must not be larger than the Trove's debt"); + require(_debtRepayment <= _currentDebt.sub(LUSD_GAS_COMPENSATION), "BorrowerOperations: Amount repaid must not be larger than the Trove's debt"); }
The same rule is also adoptable for other parts of the contracts in the Ethos-Core code base. You should follow this best practice through all of the code base.
- 　 require(!addressesSet, "Can call setAddresses only once"); + require(!addressesSet, "ActivePool: Can call setAddresses only once");
- 　_approve(sender, msg.sender, _allowances[sender][msg.sender].sub(amount, "ERC20: transfer amount exceeds allowance")); + _approve(sender, msg.sender, _allowances[sender][msg.sender].sub(amount, "LUSDToken: transfer amount exceeds allowance"));
Please consider adding more natspec documentation in the code base to improve the readability for developers.
Some very good examples of natspec documentation have been made by the developer.
import and wrong error messagesIt is not necessary to import these in the production phase.
Another minor fix to the wrong error message:
require(_maxFeePercentage <= DECIMAL_PRECISION, - "Max fee percentage must less than or equal to 100%"); + "Max fee percentage must be less than or equal to 100%");
This library is not necessary in the contract.
Always do the multiply first and then division. Follow the best practice.
-　　　　uint constant public override REDEMPTION_FEE_FLOOR = DECIMAL_PRECISION / 1000 * 5; // 0.5% -　　　　uint constant public MAX_BORROWING_FEE = DECIMAL_PRECISION / 100 * 5; // 5% + uint constant public override REDEMPTION_FEE_FLOOR = DECIMAL_PRECISION * 5 / 1000; // 0.5% // @audit-ok should multiply first then divide to follow the best practice. + uint constant public MAX_BORROWING_FEE = DECIMAL_PRECISION * 5 / 100; // 5% // @audit-ok should multiply first then divide to follow the best practice.
Ownable contractIn TroveManager contract, the ownership pattern is like this:
But in other part of the contracts, it is like this:
It is not consistent with the ownership pattern in the code base.
I suggest use the ownership pattern of inheriting the Ownable contract and call the renounceOwnership function. Do not use two different patterns in the code base.
uint256 not uintThe LQTYStaking contract is using uint not uint256. and it is bad for not following the best practice.
The LiquityMath contract should use uint256 instead of uint for following the best practice. (I understand this may be out of scope but just in case)
- using SafeMath for uint; + using SafeMath for uint256; // @audit use uint256 to follow best practice.
Throughout the Vault contracts’ code base the solidity version is as below:
It is recommended use a static version of solidity instead of a floating version in production phase.
As recommended in the slither document, 0.8.16 is a good option.
- pragma solidity ^0.8.0; + pragma soliidty 0.8.16;
The function name is balanceOf() , but the actual thing it does is to calculate the total want held by the strategy.
So a better name like totalWant() , or totalBalance() is preferred generally.
You should separate the number digits as 3 digits.
- uint256 public constant PERCENT_DIVISOR = 10000; + uint256 public constant PERCENT_DIVISOR = 10_000;
It should emits events when contract’s storage is changed.
event StrategyReported( address indexed strategy, uint256 gain, uint256 loss, uint256 debtPaid, uint256 gains, uint256 losses, uint256 allocated, uint256 allocationAdded, uint256 allocBPS ); + event TokenIsSet(address tokenAddress); + event ConstructionTime(uint256 constructionTime); + event LastReportTime(uint256 lastReportTime); + event TreasuryAdded(address index treasuryAddress); ...... /** * @notice Initializes the vault's own 'RF' token. * This token is minted when someone does a deposit. It is burned in order * to withdraw the corresponding portion of the underlying assets. * @param _token the token to maximize. * @param _name the name of the vault token. * @param _symbol the symbol of the vault token. * @param _tvlCap initial deposit cap for scaling TVL safely */ constructor( address _token, string memory _name, string memory _symbol, uint256 _tvlCap, address _treasury, address[] memory _strategists, address[] memory _multisigRoles ) ERC20(string(_name), string(_symbol)) { token = IERC20Metadata(_token); constructionTime = block.timestamp; lastReport = block.timestamp; tvlCap = _tvlCap; treasury = _treasury; lockedProfitDegradation = (DEGRADATION_COEFFICIENT * 46) / 10**6; // 6 hours in blocks uint256 numStrategists = _strategists.length; for (uint256 i = 0; i < numStrategists; i = i.uncheckedInc()) { _grantRole(STRATEGIST, _strategists[i]); } _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); _grantRole(DEFAULT_ADMIN_ROLE, _multisigRoles[0]); _grantRole(ADMIN, _multisigRoles[1]); _grantRole(GUARDIAN, _multisigRoles[2]); + emit TokenIsSet(_token); + emit ConstructionTime(block.timestamp); + emit LastReportTime(block.timestamp); + emit TvlCapUpdated(_tvlCap); + emit TreasuryAdded(_treasury); + emit LockedProfitDegradationUpdated(lockedProfitDegradation); }
The error messages below should be "Fee cannot be higher than 20 percent” or "Fee cannot be higher than 2000 BPS”, not "Fee cannot be higher than 20 BPS”.
#0 - c4-judge
2023-03-09T12:23:05Z
trust1995 marked the issue as grade-a
#1 - c4-sponsor
2023-03-20T17:17:23Z
0xBebis marked the issue as sponsor confirmed