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: 77/154
Findings: 1
Award: $61.26
🌟 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
61.2601 USDC - $61.26
| Total Low issues |
|---|
| Risk | Issues Details | Number |
|---|---|---|
| [L-01] | Low level calls with solidity version 0.8.14 and lower can result in optimiser bug | 1 |
| [L-02] | No Storage Gap for Upgradeable contracts | 2 |
| [L-03] | Loss of precision due to rounding | 10 |
| [L-04] | ERC4626Cloned 's implmentation is not fully up to EIP-4626's specification | 1 |
| [L-05] | Multiple Pragma used | All Contracts |
| [L-06] | Missing Event for initialize | 2 |
| [L-07] | Not all tokens support approve-max | 1 |
| [L-08] | Value is not validated to be different than the existing one | 2 |
| [L-09] | Use require instead of assert | 20 |
| [L-10] | Misleading comment on ActivePool.sol | 1 |
| [L-11] | Integer overflow by unsafe casting | 3 |
| Total Non-Critical issues |
|---|
| Risk | Issues Details | Number |
|---|---|---|
| [NC-01] | Include return parameters in NatSpec comments | All Contracts |
| [NC-02] | Non-usage of specific imports | All Contracts |
| [NC-03] | Lines are too long | 10 |
| [NC-04] | Lack of event emit | 6 |
| [NC-05] | Function writing does not comply with the Solidity Style Guide | All Contracts |
| [NC-06] | Solidity compiler optimizations can be problematic | 2 |
| [NC-07] | Mark visibility of initialize() functions as external | 1 |
| [NC-08] | Use bytes.concat() and string.concat() | 1 |
| [NC-09] | Reusable require statements should be changed to a modifier | 6 |
| [NC-10] | Use a more recent version of OpenZeppelin dependencies | 1 |
| [NC-11] | The protocol should include NatSpec | All Contracts |
| [NC-12] | Constants in comparisons should appear on the left side | 37 |
| [NC-13] | Use a more recent version of solidity | All Contracts |
| [NC-14] | Contracts should have full test coverage | All Contracts |
| [NC-15] | Signature Malleability of EVM's ecrecover() | 1 |
| [NC-16] | Add a timelock to critical functions | 4 |
| [NC-17] | Use immutable instead of constant for values such as a call to keccak256() | 8 |
| [NC-18] | Need Fuzzing test | All Contracts |
| [NC-19] | Generate perfect code headers every time | All Contracts |
| [NC-20] | Lock pragmas to specific compiler version | 4 |
| [NC-21] | Consider using delete rather than assigning zero to clear values | 22 |
| [NC-22] | Adding a return statement when the function defines a named return variable, is redundant | 4 |
| [NC-23] | For functions, follow Solidity standard naming conventions | All Contracts |
| [NC-24] | Events that mark critical parameter changes should contain both the old and the new value | 4 |
| [NC-25] | Add NatSpec Mapping comment | 20 |
| [NC-26] | Constants should be defined rather than using magic numbers | 6 |
| [NC-27] | There is no need to cast a variable that is already an address, such as address(x) | 1 |
| [NC-28] | Use SMTChecker | |
| [NC-29] | Not using the named return variables anywhere in the function is confusing | 16 |
| [NC-30] | Use uint256 instead uint | 477 |
| [NC-31] | Use scientific notation rather than exponentiation | 11 |
The protocol is using low level calls with solidity version 0.8.12 which can result in optimizer bug.
Ref: https://medium.com/certora/overly-optimistic-optimizer-certora-bug-disclosure-2101e3f7994d
function _chainID() private pure returns (uint256 chainID) { assembly { chainID := chainid() } }
Consider Upgrading to 0.8.15 or more.
For upgradeable contracts, inheriting contracts may introduce new variables. In order to be able to add new variables to the upgradeable contract without causing storage collisions, a storage gap should be added to the upgradeable contract.
Consider adding a storage gap at the end of the upgradeable contract:
/** * @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[50] private __gap;
Loss of precision due to the nature of arithmetics and rounding errors.
lockedProfitDegradation = (DEGRADATION_COEFFICIENT * 46) / 10**6; // 6 hours in blocks
uint256 stratMaxAllocation = (strategies[stratAddr].allocBPS * balance()) / PERCENT_DIVISOR;
uint256 vaultMaxAllocation = (totalAllocBPS * balance()) / PERCENT_DIVISOR;
return totalSupply() == 0 ? 10**decimals() : (_freeFunds() * 10**decimals()) / totalSupply();
shares = (_amount * totalSupply()) / freeFunds; // use "freeFunds" instead of "pool"
value = (_freeFunds() * _shares) / totalSupply();
totalLoss <= ((value + totalLoss) * withdrawMaxLoss) / PERCENT_DIVISOR,
return lockedProfit - ((lockedFundsRatio * lockedProfit) / DEGRADATION_COEFFICIENT);
uint256 performanceFee = (gain * strategies[strategy].feeBPS) / PERCENT_DIVISOR;
uint256 shares = supply == 0 ? performanceFee : (performanceFee * supply) / _freeFunds();
Must return the maximum amount of shares mint would allow to be deposited to receiver and not cause a revert, which must not be higher than the actual maximum that would be accepted (it should underestimate if necessary).
This assumes that the user has infinite assets, i.e. must not rely on balanceOf of asset. Could cause unexpected behavior in the future due to non-compliance with EIP-4626 standard.
function maxDeposit(address) external view override returns (uint256 maxAssets) { if (emergencyShutdown || balance() >= tvlCap) return 0; if (tvlCap == type(uint256).max) return type(uint256).max; return tvlCap - balance(); }
function maxMint(address) external view override returns (uint256 maxShares) { if (emergencyShutdown || balance() >= tvlCap) return 0; if (tvlCap == type(uint256).max) return type(uint256).max; return convertToShares(tvlCap - balance()); }
maxMint() and maxDeposit() should reflect the limitation of maxSupply.
Solidity pragma versions should be exactly same in all contracts. Currently some contracts use 0.6.11 and others use ^0.8.0
pragma solidity 0.6.11;
pragma solidity ^0.8.0;
Use one version for all contracts.
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.
function initialize( address[] calldata _collaterals, uint256[] calldata _MCRs, uint256[] calldata _CCRs ) external override onlyOwner {
function initialize( address _vault, address[] memory _strategists, address[] memory _multisigRoles, IAToken _gWant ) public initializer {
Add Event-Emit
Some tokens consider uint256 as just another value. If one of these tokens is used and the approval balance eventually reaches only a couple wei, nobody will be able to send funds because there won't be enough approval.
IERC20Upgradeable(want).safeApprove(vault, type(uint256).max);
The updateCollateralRatios() function is an admin function to update _MCR and _CCR, it must only lower the values as stated in the comments:
// Admin function to lower the collateralization requirements for a particular collateral. // Can only lower, not increase.
but in the bellow checks:
require(_MCR <= config.MCR, "Can only walk down the MCR"); require(_CCR <= config.CCR, "Can only walk down the CCR");
the owner can also set the values to be the same as before which will waste a Gcoldsload of gas and cause multiple abnormal events to be emitted, will ultimately result in a no-op situation.
function updateCollateralRatios( address _collateral, uint256 _MCR, uint256 _CCR ) external onlyOwner checkCollateral(_collateral) { Config storage config = collateralConfig[_collateral]; require(_MCR < config.MCR, "Can only walk down the MCR"); require(_CCR < config.CCR, "Can only walk down the CCR"); require(_MCR >= MIN_ALLOWED_MCR, "MCR below allowed minimum"); config.MCR = _MCR; require(_CCR >= MIN_ALLOWED_CCR, "CCR below allowed minimum"); config.CCR = _CCR; emit CollateralRatiosUpdated(_collateral, _MCR, _CCR); }
Assert should not be used except for tests, require should be used
Prior to Solidity 0.8.0, pressing a confirm consumes the remainder of the process's available gas instead of returning it, as request()/revert() did.
The big difference between the two is that the assert() function when false, uses up all the remaining gas and reverts all the changes made.
Meanwhile, a require() statement when false, also reverts back all the changes made to the contract but does refund all the remaining gas fees we offered to pay. This is the most common Solidity function used by developers for debugging and error handling.
Assertion should be avoided even after solidity version 0.8.0, because its documentation states "The Assert function generates an error of type Panic(uint256). Code that works properly should never Panic, even on invalid external input. If this happens, you need to fix it in your contract. there's a mistake".
assert(MIN_NET_DEBT > 0);
assert(vars.compositeDebt > 0);
assert(msg.sender == _borrower || (msg.sender == stabilityPoolAddress && _collTopUp > 0 && _LUSDChange == 0));
assert(_collWithdrawal <= vars.coll);
assert(_LUSDInStabPool != 0);
assert(totalStakesSnapshot[_collateral] > 0);
assert(closedStatus != Status.nonExistent && closedStatus != Status.active);
assert(troveStatus != Status.nonExistent && troveStatus != Status.active);
assert(index <= idxLast);
assert(newBaseRate > 0); // Base rate is always non-zero after redemption
assert(decayedBaseRate <= DECIMAL_PRECISION); // The baseRate can decay to 0
assert(_debtToOffset <= _totalLUSDDeposits);
assert(_LUSDLossPerUnitStaked <= DECIMAL_PRECISION);
assert(newP > 0);
assert(sender != address(0));
assert(recipient != address(0));
assert(account != address(0));
assert(account != address(0));
assert(owner != address(0));
assert(spender != address(0));
Use require() instead of assert()
ActivePool.solThere is a misleading comment on the ActivePool.sol that may mislead some users or auditors in the future:
// if abs(percentOfFinalBal - yieldingPercentage) > drift, we will need to deposit more or withdraw some vars.yieldingPercentage = yieldingPercentage[_collateral]; vars.finalYieldingAmount = vars.finalBalance.mul(vars.yieldingPercentage).div(10_000); vars.yieldingAmount = yieldingAmount[_collateral]; if (vars.percentOfFinalBal > vars.yieldingPercentage && vars.percentOfFinalBal.sub(vars.yieldingPercentage) > yieldingPercentageDrift) { // we will end up overallocated, withdraw some vars.toWithdraw = vars.currentAllocated.sub(vars.finalYieldingAmount); vars.yieldingAmount = vars.yieldingAmount.sub(vars.toWithdraw); yieldingAmount[_collateral] = vars.yieldingAmount; } else if(vars.percentOfFinalBal < vars.yieldingPercentage && vars.yieldingPercentage.sub(vars.percentOfFinalBal) > yieldingPercentageDrift) { // we will end up underallocated, deposit more vars.toDeposit = vars.finalYieldingAmount.sub(vars.currentAllocated); vars.yieldingAmount = vars.yieldingAmount.add(vars.toDeposit); yieldingAmount[_collateral] = vars.yieldingAmount; }
As you can see in the above code, if abs(percentOfFinalBal - yieldingPercentage) > drift we will withdraw some and only if abs(yieldingPercentage - percentOfFinalBal) > drift we will need to deposit more.
// if abs(percentOfFinalBal - yieldingPercentage) > drift, we will need to deposit more or withdraw some
Change the NatSpec to :
// if abs(percentOfFinalBal - yieldingPercentage) > drift, we will need to withdraw some vars.yieldingPercentage = yieldingPercentage[_collateral]; vars.finalYieldingAmount = vars.finalBalance.mul(vars.yieldingPercentage).div(10_000); vars.yieldingAmount = yieldingAmount[_collateral]; if (vars.percentOfFinalBal > vars.yieldingPercentage && vars.percentOfFinalBal.sub(vars.yieldingPercentage) > yieldingPercentageDrift) { // we will end up overallocated, withdraw some vars.toWithdraw = vars.currentAllocated.sub(vars.finalYieldingAmount); vars.yieldingAmount = vars.yieldingAmount.sub(vars.toWithdraw); yieldingAmount[_collateral] = vars.yieldingAmount; // if abs(yieldingPercentage - percentOfFinalBal) > drift, we will need to deposit more } else if(vars.percentOfFinalBal < vars.yieldingPercentage && vars.yieldingPercentage.sub(vars.percentOfFinalBal) > yieldingPercentageDrift) { // we will end up underallocated, deposit more vars.toDeposit = vars.finalYieldingAmount.sub(vars.currentAllocated); vars.yieldingAmount = vars.yieldingAmount.add(vars.toDeposit); yieldingAmount[_collateral] = vars.yieldingAmount; }
Integer overflow by unsafe casting. It is necessary to safely convert between the different numeric types.
vars.netAssetMovement = int256(vars.toDeposit) - int256(vars.toWithdraw) - int256(vars.profit);
Use OpenZeppelin safeCast library.
If Return parameters are declared, you must prefix them with /// @return.
Some code analysis programs do analysis by reading NatSpec details, if they can't see the @return tag, they do incomplete analysis.
Include @return parameters in NatSpec comments
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.
Use specific imports syntax per solidity docs recommendation.
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length.
Ref: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length
function openTrove(address _collateral, uint _collAmount, uint _maxFeePercentage, uint _LUSDAmount, address _upperHint, address _lowerHint) external override {
function adjustTrove(address _collateral, uint _maxFeePercentage, uint _collTopUp, uint _collWithdrawal, uint _LUSDChange, bool _isDebtIncrease, address _upperHint, address _lowerHint) external override {
function _adjustTrove(address _borrower, address _collateral, uint _collTopUp, uint _collWithdrawal, uint _LUSDChange, bool _isDebtIncrease, address _upperHint, address _lowerHint, uint _maxFeePercentage) internal {
(vars.newColl, vars.newDebt) = _updateTroveFromAdjustment(contractsCache.troveManager, _borrower, _collateral, vars.collChange, vars.isCollIncrease, vars.netDebtChange, _isDebtIncrease);
emit TroveLiquidated(_borrower, _collateral, singleLiquidation.entireTroveDebt, singleLiquidation.entireTroveColl, TroveManagerOperation.liquidateInNormalMode);
emit TroveLiquidated(_borrower, _collateral, singleLiquidation.entireTroveDebt, singleLiquidation.entireTroveColl, TroveManagerOperation.liquidateInRecoveryMode);
emit TroveLiquidated(_borrower, _collateral, singleLiquidation.entireTroveDebt, singleLiquidation.entireTroveColl, TroveManagerOperation.liquidateInRecoveryMode);
emit TroveLiquidated(_borrower, _collateral, singleLiquidation.entireTroveDebt, singleLiquidation.collToSendToSP, TroveManagerOperation.liquidateInRecoveryMode);
* The redeemer swaps (debt - liquidation reserve) LUSD for (debt - liquidation reserve) worth of collateral, so the LUSD liquidation reserve left corresponds to the remaining debt.
// Return the nominal collateral ratio (ICR) of a given Trove, without the price. Takes a trove's pending coll and debt rewards from redistributions into account.
The below methods do not emit an event when the state changes, something that it's very important for dApps and users.
function updateDistributionPeriod(uint256 _newDistributionPeriod) external onlyOwner { distributionPeriod = _newDistributionPeriod; }
function sendOath(address _account, uint _OathAmount) external override { _requireCallerIsStabilityPool(); OathToken.transfer(_account, _OathAmount); }
function setTroveStatus(address _borrower, address _collateral, uint _num) external override { _requireCallerIsBorrowerOperations(); Troves[_borrower][_collateral].status = Status(_num); }
function updateTreasury(address newTreasury) external { _atLeastRole(DEFAULT_ADMIN_ROLE); require(newTreasury != address(0), "Invalid address"); treasury = newTreasury; }
function setEmergencyExit() external { _atLeastRole(GUARDIAN); emergencyExit = true; IVault(vault).revokeStrategy(address(this)); }
function setHarvestSteps(address[2][] calldata _newSteps) external { _atLeastRole(ADMIN); delete steps; uint256 numSteps = _newSteps.length; for (uint256 i = 0; i < numSteps; i = i.uncheckedInc()) { address[2] memory step = _newSteps[i]; require(step[0] != address(0)); require(step[1] != address(0)); steps.push(step); } }
Event-emit.
Solidity Style GuideOrdering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.
Functions should be grouped according to their visibility and ordered:
constructor()receive()fallback()external / public / internal / privateview / pureFollow Solidity Style Guide.
Protocol has enabled optional compiler optimizations in Solidity. There have been several optimization bugs with security implications. Moreover, optimizations are actively being developed. Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them.
Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past. A high-severity bug in the emscripten-generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG.
Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. More recently, another bug due to the incorrect caching of keccak256 was reported. A compiler audit of Solidity from November 2018 concluded that the optional optimizations may not be safe. It is likely that there are latent bugs related to optimization and that new bugs will be introduced due to future optimizations.
Exploit Scenario A latent or future bug in Solidity compiler optimizations—or in the Emscripten transpilation to solc-js—causes a security vulnerability in the contracts.
settings: { optimizer: { enabled: true, runs: 200, },
version: "0.4.23", settings: { optimizer: { enabled: true, runs: 100 } } }, { version: "0.5.17", settings: { optimizer: { enabled: true, runs: 100 } } }, { version: "0.6.11", settings: { optimizer: { enabled: true, runs: 100 } }
Short term, measure the gas savings from optimizations and carefully weigh them against the possibility of an optimization-related bug. Long term, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.
initialize() functions as externalIf someone wants to extend via inheritance, it might make more sense that the overridden initialize() function calls the internal {...}_init function, not the parent public initialize() function.
External instead of public would give more the sense of the initialize() functions to behave like a constructor (only called on deployment, so should only be called externally)
Security point of view, it might be safer so that it cannot be called internally by accident in the child contract
It might cost a bit less gas to use external over public
It is possible to override a function from external to public ("opening it up") but it is not possible to override a function from public to external ("narrow it down").
Ref: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3750
function initialize( address _vault, address[] memory _strategists, address[] memory _multisigRoles, IAToken _gWant ) public initializer { gWant = _gWant; want = _gWant.UNDERLYING_ASSET_ADDRESS(); __ReaperBaseStrategy_init(_vault, want, _strategists, _multisigRoles); rewardClaimingTokens = [address(_gWant)]; }
Change the visibility of initialize() functions to external
bytes.concat() and string.concat()Solidity version 0.8.4 introduces:
bytes.concat() vs abi.encodePacked(<bytes>,<bytes>)string.concat() vs abi.encodePacked(<string>,<string>)bytes32 digest = keccak256(abi.encodePacked('\x19\x01', domainSeparator(), keccak256(abi.encode( _PERMIT_TYPEHASH, owner, spender, amount, _nonces[owner]++, deadline))));
Use bytes.concat() and string.concat()
Reusable require statements should be changed to a modifier.
require(_MCRs[i] >= MIN_ALLOWED_MCR, "MCR below allowed minimum");
require(_MCR >= MIN_ALLOWED_MCR, "MCR below allowed minimum");
require(_CCRs[i] >= MIN_ALLOWED_CCR, "CCR below allowed minimum");
require(_CCR >= MIN_ALLOWED_CCR, "CCR below allowed minimum");
require(_feeBPS <= PERCENT_DIVISOR / 5, "Fee cannot be higher than 20 BPS");
require(_feeBPS <= PERCENT_DIVISOR / 5, "Fee cannot be higher than 20 BPS");
Use modifiers.
For security, it is best practice to use the latest OpenZeppelin version.
"dependencies": { "@openzeppelin/contracts": "^4.7.3", "@openzeppelin/contracts-upgradeable": "^4.7.3", "dotenv": "^16.0.1" },
Old version of OpenZeppelin is used (4.7.3), newer version can be used (4.8.0).
It is recommended that Solidity contracts are fully annotated using NatSpec, it is clearly stated in the Solidity official documentation.
In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.
Some code analysis programs do analysis by reading NatSpec details, if they can't see the tags (@param, @dev, @return), they do incomplete analysis.
Include NatSpec comments in the codebase.
Constants in comparisons should appear on the left side, doing so will prevent typo bug.
function _requireSingularCollChange(uint _collTopUp, uint _collWithdrawal) internal pure { require(_collTopUp == 0 || _collWithdrawal == 0, "BorrowerOperations: Cannot withdraw and add coll"); }
Lines: 301, 534, 568
Lines: 428, 469, 489, 475, 685, 720, 751, 784, 795, 809
Lines: 616, 817, 1127, 1142, 1215, 1238
Lines: 152, 210, 227, 296, 331, 380, 466, 555
Lines: 52, 67, 140, 184
Lines: 121
function _requireSingularCollChange(uint _collTopUp, uint _collWithdrawal) internal pure { require(0 == _collTopUp || 0 == _collWithdrawal, "BorrowerOperations: Cannot withdraw and add coll"); }
For security, it is best practice to use the latest Solidity version.
pragma solidity 0.6.11;
pragma solidity ^0.8.0;
Old versions of Solidity are used (0.8.0), (0.6.11), newer version can be used (0.8.17).
While 100% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.
- What is the overall line coverage percentage provided by your tests?: 93
Line coverage percentage should be 100%.
ecrecover()The function calls the Solidity ecrecover() function directly to verify the given signatures. However, the ecrecover() EVM opcode allows malleable (non-unique) signatures and thus is susceptible to replay attacks.
Although a replay attack seems not possible for this contract, I recommend using the battle-tested OpenZeppelin's ECDSA library.
address recoveredAddress = ecrecover(digest, v, r, s);
Use the ecrecover function from OpenZeppelin's ECDSA library for signature verification. (Ensure using a version > 4.7.3 for there was a critical bug >= 4.1.0 < 4.7.3).
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.
function setYieldingPercentage(address _collateral, uint256 _bps) external onlyOwner { _requireValidCollateralAddress(_collateral); require(_bps <= 10_000, "Invalid BPS value"); yieldingPercentage[_collateral] = _bps; emit YieldingPercentageUpdated(_collateral, _bps); }
function setYieldingPercentageDrift(uint256 _driftBps) external onlyOwner { require(_driftBps <= 500, "Exceeds max allowed value of 500 BPS"); yieldingPercentageDrift = _driftBps; emit YieldingPercentageDriftUpdated(_driftBps); }
function setYieldClaimThreshold(address _collateral, uint256 _threshold) external onlyOwner { _requireValidCollateralAddress(_collateral); yieldClaimThreshold[_collateral] = _threshold; emit YieldClaimThresholdUpdated(_collateral, _threshold); }
function setYieldDistributionParams(uint256 _treasurySplit, uint256 _SPSplit, uint256 _stakingSplit) external onlyOwner { require(_treasurySplit + _SPSplit + _stakingSplit == 10_000, "Splits must add up to 10000 BPS"); yieldSplitTreasury = _treasurySplit; yieldSplitSP = _SPSplit; yieldSplitStaking = _stakingSplit; emit YieldDistributionParamsUpdated(_treasurySplit, _SPSplit, _stakingSplit); }
Consider adding a timelock to the critical changes.
immutable instead of constant for values such as a call to keccak256()While it doesn't save any gas because the compiler knows that developers often make this mistake, it's still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.
bytes32 public constant DEPOSITOR = keccak256("DEPOSITOR");
bytes32 public constant STRATEGIST = keccak256("STRATEGIST");
bytes32 public constant GUARDIAN = keccak256("GUARDIAN");
bytes32 public constant ADMIN = keccak256("ADMIN");
bytes32 public constant KEEPER = keccak256("KEEPER");
bytes32 public constant STRATEGIST = keccak256("STRATEGIST");
bytes32 public constant GUARDIAN = keccak256("GUARDIAN");
bytes32 public constant ADMIN = keccak256("ADMIN");
Use immutable instead of constants
Use should fuzzing test like Echidna. As Alberto Cuesta Canada said:
Fuzzing is not easy, the tools are rough, and the math is hard, but it is worth it. Fuzzing gives me a level of confidence in my smart contracts that I didn’t have before. Relying just on unit testing anymore and poking around in a testnet seems reckless now.
Ref: https://medium.com/coinmonks/smart-contract-fuzzing-d9b88e0b0a05
I recommend using header for Solidity code layout and readability
/*////////////////////////////////////////////////////////////// TESTING 123 //////////////////////////////////////////////////////////////*/
Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile locally.
pragma solidity ^0.8.0;
pragma solidity ^0.8.0;
pragma solidity ^0.8.0;
pragma solidity ^0.8.0;
Ethereum Smart Contract Best Practices: Lock pragmas to specific compiler version.
delete rather than assigning zero to clear valuesThe delete keyword more closely matches the semantics of what is being done, and draws more attention to the changing of state, which may lead to a more thorough audit of its associated logic.
Lines: 253
Lines: 529, 578, 756
Lines: 387, 388, 468, 469, 505, 506, 1192, 1285, 1286, 1288, 1289
Lines: 215, 370, 372, 538
Lines: 100, 112, 117
Use the delete keyword.
return statement when the function defines a named return variable, is redundantAdding a return statement when the function defines a named return variable, is redundant.
return (collGainPerUnitStaked, LUSDLossPerUnitStaked);
return singleLiquidation;
return singleLiquidation;
return index;
The protocol don't follow solidity standard naming convention.
Ref: https://docs.soliditylang.org/en/v0.8.17/style-guide.html#naming-conventions
Follow solidity standard naming convention.
Events that mark critical parameter changes should contain both the old and the new value.
emit YieldingPercentageUpdated(_collateral, _bps);
emit YieldingPercentageDriftUpdated(_driftBps);
emit YieldClaimThresholdUpdated(_collateral, _threshold);
emit YieldDistributionParamsUpdated(_treasurySplit, _SPSplit, _stakingSplit);
Add the old value to the event.
Add NatSpec comments describing mapping keys and values.
mapping (address => uint256) private _nonces;
Lines: 35
Lines: 54, 57, 58, 63, 64, 65
Lines: 167, 179
Lines: 88, 91, 94, 104, 105, 119, 120
Lines: 28, 32, 35
/// @dev address(owner) => uint(nonce) mapping (address => uint256) private _nonces;
Constants should be defined rather than using magic numbers.
require(_bps <= 10_000, "Invalid BPS value");
require(_treasurySplit + _SPSplit + _stakingSplit == 10_000, "Splits must add up to 10000 BPS");
vars.percentOfFinalBal = vars.finalBalance == 0 ? uint256(-1) : vars.currentAllocated.mul(10_000).div(vars.finalBalance);
vars.finalYieldingAmount = vars.finalBalance.mul(vars.yieldingPercentage).div(10_000);
vars.treasurySplit = vars.profit.mul(yieldSplitTreasury).div(10_000);
vars.stakingSplit = vars.profit.mul(yieldSplitStaking).div(10_000);
Define a constant.
address(x)There is no need to cast a variable that is already an address, such as address(x), x is also address.
(uint256 supply, , , , , , , , ) = IAaveProtocolDataProvider(DATA_PROVIDER).getUserReserveData( address(want), address(this) );
(uint256 supply, , , , , , , , ) = IAaveProtocolDataProvider(DATA_PROVIDER).getUserReserveData( want, address(this) );
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.
Ref: https://twitter.com/0xOwenThurm/status/1614359896350425088?t=dbG9gHFigBX85Rv29lOjIQ&s=19
Use SMTChecker
Not using the named return variables anywhere in the function is confusing.
Lines: 626
Lines: 1316
Lines: 89, 104
Lines: 29, 37, 51, 66, 79, 96, 110, 122, 165, 220, 240, 269
Consider changing the variable to be an unnamed one.
uint256 instead uint477 results in 8 files.
Some developers prefer to use uint256 because it is consistent with other uint data types, which also specify their size, and also because making the size of the data explicit reminds the developer and the reader how much data they've got to play with, which may help prevent or detect bugs.
For example if doing bytes4(keccak('transfer(address, uint)’)), you'll get a different method sig ID than bytes4(keccak('transfer(address, uint256)’)) and smart contracts will only understand the latter when comparing method sig IDs.
Use uint256 instead uint.
While the compiler knows to optimize away the exponentiation, it's still better coding practice to use idioms that do not require compiler optimization, if they exist.
uint usdValue = _price.mul(_coll).div(10**_collDecimals);
cappedCollPortion = cappedCollPortion.div(10 ** (LiquityMath.CR_CALCULATION_DECIMALS - _collDecimals));
cappedCollPortion = cappedCollPortion.mul(10 ** (_collDecimals - LiquityMath.CR_CALCULATION_DECIMALS));
uint pendingCollateralReward = stake.mul(rewardPerUnitStaked).div(10**collDecimals);
uint pendingLUSDDebtReward = stake.mul(rewardPerUnitStaked).div(10**collDecimals);
uint collateralNumerator = _coll.mul(10**_collDecimals).add(lastCollateralError_Redistribution[_collateral]);
uint LUSDDebtNumerator = _debt.mul(10**_collDecimals).add(lastLUSDDebtError_Redistribution[_collateral]);
uint256 public constant DEGRADATION_COEFFICIENT = 10**18; // The unit for calculating profit degradation.
lockedProfitDegradation = (DEGRADATION_COEFFICIENT * 46) / 10**6; // 6 hours in blocks
return totalSupply() == 0 ? 10**decimals() : (_freeFunds() * 10**decimals()) / totalSupply();
Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18).
#0 - c4-judge
2023-03-10T14:39:57Z
trust1995 marked the issue as grade-a
#1 - c4-judge
2023-03-10T17:37:26Z
trust1995 marked the issue as grade-b
#2 - c4-sponsor
2023-03-28T21:41:14Z
0xBebis marked the issue as sponsor confirmed