Platform: Code4rena
Start Date: 11/01/2023
Pot Size: $60,500 USDC
Total HM: 6
Participants: 69
Period: 6 days
Judge: Trust
Total Solo HM: 2
Id: 204
League: ETH
Rank: 15/69
Findings: 2
Award: $336.94
š Selected for report: 0
š Solo Findings: 0
š Selected for report: CodingNameKiki
Also found by: 0x1f8b, 0x52, 0x5rings, 0xAgro, 0xSmartContract, 0xcm, 0xkato, 2997ms, Aymen0909, BClabs, BPZ, BRONZEDISC, Bauer, Bnke0x0, Deekshith99, IllIllI, Josiah, Kaysoft, RaymondFam, Rolezn, SaeedAlipoor01988, Tajobin, Udsen, Viktor_Cortess, adriro, arialblack14, betweenETHlines, btk, chaduke, chrisdior4, cryptphi, csanuragjain, cygaar, defsec, descharre, erictee, gzeon, hansfriese, horsefacts, joestakey, koxuan, lukris02, luxartvinsec, nicobevi, oyc_109, pavankv, peanuts, rbserver, scokaf, shark, tnevler, tsvetanovv, zaskoh
304.5822 USDC - $304.58
| Issue | Instances | |
|---|---|---|
| [Lā01] | Upgradeable contract not initialized | 1 |
| [Lā02] | Loss of precision | 1 |
| [Lā03] | Owner can renounce while system is paused | 1 |
| [Lā04] | require() should be used instead of assert() | 3 |
Total: 6 instances over 4 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions | 4 |
| [Nā02] | Import declarations should import specific identifiers, rather than the whole file | 45 |
| [Nā03] | Duplicate import statements | 1 |
| [Nā04] | The nonReentrant modifier should occur before all other modifiers | 3 |
| [Nā05] | Adding a return statement when the function defines a named return variable, is redundant | 1 |
| [Nā06] | constants should be defined rather than using magic numbers | 3 |
| [Nā07] | Numeric values having to do with time should use time units for readability | 1 |
| [Nā08] | Events that mark critical parameter changes should contain both the old and the new value | 2 |
| [Nā09] | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant | 14 |
| [Nā10] | Constant redefined elsewhere | 7 |
| [Nā11] | Typos | 5 |
| [Nā12] | Constructor visibility is ignored | 3 |
| [Nā13] | File is missing NatSpec | 1 |
| [Nā14] | NatSpec is incomplete | 16 |
| [Nā15] | Consider using delete rather than assigning zero to clear values | 3 |
| [Nā16] | Contracts should have full test coverage | 1 |
| [Nā17] | Large or complicated code bases should implement fuzzing tests | 1 |
| [Nā18] | Function ordering does not follow the Solidity style guide | 8 |
| [Nā19] | Contract does not follow the Solidity style guide's suggested layout ordering | 14 |
| [Nā20] | Interfaces should be indicated with an I prefix in the contract name | 6 |
Total: 139 instances over 20 issues
Upgradeable contracts are initialized via an initializer function rather than by a constructor. Leaving such a contract uninitialized may lead to it being taken over by a malicious user
There is 1 instance of this issue:
File: contracts/cash/token/Cash.sol /// @audit missing __ERC20PresetMinterPauser_init() 21: contract Cash is ERC20PresetMinterPauserUpgradeable {
Division by large numbers may result in the result being zero, due to solidity not supporting fractions. Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator
There is 1 instance of this issue:
File: contracts/cash/CashManager.sol 755 uint256 collateralAmountDue = (amountToDist * cashAmountReturned) / 756: quantityBurned;
The contract owner or single user with a role is not prevented from renouncing the role/ownership while the contract is paused, which would cause any user assets stored in the protocol, to be locked indefinitely
There is 1 instance of this issue:
File: contracts/cash/CashManager.sol 526 function pause() external onlyRole(PAUSER_ADMIN) { 527 _pause(); 528: }
require() should be used instead of assert()Prior to solidity version 0.8.0, hitting an assert consumes the remainder of the transaction's available gas rather than returning it, as require()/revert() do. assert() should be avoided even past solidity version 0.8.0 as its documentation states that "The assert function creates an error of type Panic(uint256). ... Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix".
There are 3 instances of this issue:
File: contracts/cash/factory/CashFactory.sol 97: assert(cashProxyAdmin.owner() == guardian);
File: contracts/cash/factory/CashKYCSenderFactory.sol 106: assert(cashKYCSenderProxyAdmin.owner() == guardian);
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol 106: assert(cashKYCSenderReceiverProxyAdmin.owner() == guardian);
__gap[50] storage variable to allow for new storage variables in later versionsSee this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.
There are 4 instances of this issue:
File: contracts/cash/Proxy.sol 20: contract TokenProxy is TransparentUpgradeableProxy {
File: contracts/cash/token/CashKYCSenderReceiver.sol 22 contract CashKYCSenderReceiver is 23 ERC20PresetMinterPauserUpgradeable, 24: KYCRegistryClientInitializable
File: contracts/cash/token/CashKYCSender.sol 22 contract CashKYCSender is 23 ERC20PresetMinterPauserUpgradeable, 24: KYCRegistryClientInitializable
File: contracts/cash/token/Cash.sol 21: contract Cash is ERC20PresetMinterPauserUpgradeable {
Using import declarations of the form import {<identifier_name>} from "some/file.sol" avoids polluting the symbol namespace making flattened files smaller, and speeds up compilation
There are 45 instances of this issue:
File: contracts/cash/CashManager.sol 17: import "contracts/cash/interfaces/ICashManager.sol"; 18: import "contracts/cash/interfaces/IMulticall.sol"; 19: import "contracts/cash/token/Cash.sol"; 20: import "contracts/cash/kyc/KYCRegistryClientConstructable.sol"; 21: import "contracts/cash/external/openzeppelin/contracts/security/Pausable.sol"; 22: import "contracts/cash/external/openzeppelin/contracts/token/IERC20.sol"; 23: import "contracts/cash/external/openzeppelin/contracts/token/IERC20Metadata.sol"; 24: import "contracts/cash/external/openzeppelin/contracts/token/SafeERC20.sol"; 25: import "contracts/cash/external/openzeppelin/contracts/access/AccessControlEnumerable.sol"; 26: import "contracts/cash/external/openzeppelin/contracts/security/ReentrancyGuard.sol";
File: contracts/cash/factory/CashFactory.sol 19: import "contracts/cash/external/openzeppelin/contracts/proxy/ProxyAdmin.sol"; 20: import "contracts/cash/Proxy.sol"; 21: import "contracts/cash/token/Cash.sol"; 22: import "contracts/cash/interfaces/IMulticall.sol";
File: contracts/cash/factory/CashKYCSenderFactory.sol 19: import "contracts/cash/external/openzeppelin/contracts/proxy/ProxyAdmin.sol"; 20: import "contracts/cash/Proxy.sol"; 21: import "contracts/cash/token/CashKYCSender.sol"; 22: import "contracts/cash/interfaces/IMulticall.sol";
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol 19: import "contracts/cash/external/openzeppelin/contracts/proxy/ProxyAdmin.sol"; 20: import "contracts/cash/Proxy.sol"; 21: import "contracts/cash/token/CashKYCSenderReceiver.sol"; 22: import "contracts/cash/interfaces/IMulticall.sol";
File: contracts/cash/interfaces/IKYCRegistryClient.sol 18: import "contracts/cash/interfaces/IKYCRegistry.sol";
File: contracts/cash/kyc/KYCRegistryClientConstructable.sol 17: import "contracts/cash/kyc/KYCRegistryClient.sol"; 21: import "contracts/cash/kyc/KYCRegistryClient.sol";
File: contracts/cash/kyc/KYCRegistryClientInitializable.sol 17: import "contracts/cash/kyc/KYCRegistryClient.sol"; 18: import "contracts/cash/external/openzeppelin/contracts-upgradeable/proxy/Initializable.sol";
File: contracts/cash/kyc/KYCRegistryClient.sol 17: import "contracts/cash/interfaces/IKYCRegistry.sol"; 18: import "contracts/cash/interfaces/IKYCRegistryClient.sol";
File: contracts/cash/kyc/KYCRegistry.sol 18: import "contracts/cash/interfaces/IKYCRegistry.sol"; 19: import "contracts/cash/external/openzeppelin/contracts/access/AccessControlEnumerable.sol"; 20: import "contracts/cash/external/chainalysis/ISanctionsList.sol"; 21: import "contracts/cash/external/openzeppelin/contracts/utils/cryptography/EIP712.sol"; 22: import "contracts/cash/external/openzeppelin/contracts/utils/cryptography/ECDSA.sol";
File: contracts/cash/Proxy.sol 18: import "contracts/cash/external/openzeppelin/contracts/proxy/TransparentUpgradeableProxy.sol";
File: contracts/cash/token/CashKYCSenderReceiver.sol 18: import "contracts/cash/external/openzeppelin/contracts-upgradeable/token/ERC20/ERC20PresetMinterPauserUpgradeable.sol"; 19: import "contracts/cash/kyc/KYCRegistryClientInitializable.sol";
File: contracts/cash/token/CashKYCSender.sol 18: import "contracts/cash/external/openzeppelin/contracts-upgradeable/token/ERC20/ERC20PresetMinterPauserUpgradeable.sol"; 19: import "contracts/cash/kyc/KYCRegistryClientInitializable.sol";
File: contracts/cash/token/Cash.sol 18: import "contracts/cash/external/openzeppelin/contracts-upgradeable/token/ERC20/ERC20PresetMinterPauserUpgradeable.sol";
File: contracts/lending/OndoPriceOracle.sol 17: import "./IOndoPriceOracle.sol"; 18: import "contracts/lending/compound/Ownable.sol";
File: contracts/lending/OndoPriceOracleV2.sol 17: import "./IOndoPriceOracleV2.sol"; 18: import "contracts/cash/external/openzeppelin/contracts/access/Ownable.sol"; 19: import "contracts/lending/chainlink/AggregatorV3Interface.sol";
There is 1 instance of this issue:
File: contracts/cash/kyc/KYCRegistryClientConstructable.sol 21: import "contracts/cash/kyc/KYCRegistryClient.sol";
nonReentrant modifier should occur before all other modifiersThis is a best-practice to protect against reentrancy in other modifiers
There are 3 instances of this issue:
File: contracts/cash/CashManager.sol 201: nonReentrant 244: ) external override updateEpoch nonReentrant whenNotPaused checkKYC(user) { 668: nonReentrant
return statement when the function defines a named return variable, is redundantThere is 1 instance of this issue:
File: contracts/cash/CashManager.sol 795: return totalCashAmountRefunded;
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 3 instances of this issue:
File: contracts/cash/CashManager.sol /// @audit 1e6 491: uint256 amountE24 = _scaleUp(collateralAmountIn) * 1e6;
File: contracts/cash/kyc/KYCRegistry.sol /// @audit 27 /// @audit 28 87: require(v == 27 || v == 28, "KYCRegistry: invalid v value in signature");
There are units for seconds, minutes, hours, days, and weeks, and since they're defined, they should be used
There is 1 instance of this issue:
File: contracts/lending/OndoPriceOracleV2.sol /// @audit 90000 77: uint256 public maxChainlinkOracleTimeDelay = 90000; // 25 hours
This should especially be done if the new value is not required to be different from the old value
There are 2 instances of this issue:
File: contracts/cash/CashManager.sol /// @audit setPendingRedemptionBalance() 870 emit PendingRedemptionBalanceSet( 871 user, 872 epoch, 873 balance, 874 redemptionInfoPerEpoch[epoch].totalBurned 875: );
File: contracts/lending/OndoPriceOracleV2.sol /// @audit setFTokenToOracleType() 150: emit FTokenToOracleTypeSet(fToken, oracleType);
keccak256(), should use immutable rather than constantWhile 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.
There are 14 instances of this issue:
File: contracts/cash/CashManager.sol 122: bytes32 public constant MANAGER_ADMIN = keccak256("MANAGER_ADMIN"); 123: bytes32 public constant PAUSER_ADMIN = keccak256("PAUSER_ADMIN"); 124: bytes32 public constant SETTER_ADMIN = keccak256("SETTER_ADMIN");
File: contracts/cash/factory/CashFactory.sol 44: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE"); 45: bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
File: contracts/cash/factory/CashKYCSenderFactory.sol 44: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE"); 45: bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol 44: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE"); 45: bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
File: contracts/cash/kyc/KYCRegistry.sol 31 bytes32 public constant _APPROVAL_TYPEHASH = 32 keccak256( 33 "KYCApproval(uint256 kycRequirementGroup,address user,uint256 deadline)" 34: ); 36: bytes32 public constant REGISTRY_ADMIN = keccak256("REGISTRY_ADMIN");
File: contracts/cash/token/CashKYCSenderReceiver.sol 26 bytes32 public constant KYC_CONFIGURER_ROLE = 27: keccak256("KYC_CONFIGURER_ROLE");
File: contracts/cash/token/CashKYCSender.sol 26 bytes32 public constant KYC_CONFIGURER_ROLE = 27: keccak256("KYC_CONFIGURER_ROLE");
File: contracts/cash/token/Cash.sol 22: bytes32 public constant TRANSFER_ROLE = keccak256("TRANSFER_ROLE");
Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync.
There are 7 instances of this issue:
File: contracts/cash/factory/CashKYCSenderFactory.sol /// @audit seen in contracts/cash/factory/CashFactory.sol 44: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE"); /// @audit seen in contracts/cash/factory/CashFactory.sol 45: bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE"); /// @audit seen in contracts/cash/factory/CashFactory.sol 46: bytes32 public constant DEFAULT_ADMIN_ROLE = bytes32(0);
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol /// @audit seen in contracts/cash/factory/CashKYCSenderFactory.sol 44: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE"); /// @audit seen in contracts/cash/factory/CashKYCSenderFactory.sol 45: bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE"); /// @audit seen in contracts/cash/factory/CashKYCSenderFactory.sol 46: bytes32 public constant DEFAULT_ADMIN_ROLE = bytes32(0);
File: contracts/cash/token/CashKYCSender.sol /// @audit seen in contracts/cash/token/CashKYCSenderReceiver.sol 26 bytes32 public constant KYC_CONFIGURER_ROLE = 27: keccak256("KYC_CONFIGURER_ROLE");
There are 5 instances of this issue:
File: contracts/cash/interfaces/IKYCRegistryClient.sol /// @audit refernce 38: /// @notice Error for when caller attempts to set the KYC registry refernce
File: contracts/cash/kyc/KYCRegistry.sol /// @audit sucessfully 62: * `kycRequirementGroup`. In order to sucessfully call this function, /// @audit elligible 205: * @param addresses Array of addresses being added as elligible /// @audit elligible 236: * @param addresses Array of addresses being added as elligible
File: contracts/lending/IOndoPriceOracleV2.sol /// @audit assset 95: * @dev Event for when a price cap is set on an fToken's underlying assset
Remove the public if using solidity at or above 0.7.0
There are 3 instances of this issue:
File: contracts/cash/factory/CashFactory.sol 53: constructor(address _guardian) {
File: contracts/cash/factory/CashKYCSenderFactory.sol 53: constructor(address _guardian) {
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol 53: constructor(address _guardian) {
There is 1 instance of this issue:
File: contracts/cash/Proxy.sol
There are 16 instances of this issue:
File: contracts/cash/CashManager.sol /// @audit Missing: '@return' 500 * (in decimals of `collateral`) 501 */ 502 function _getMintFees( 503 uint256 collateralAmount 504: ) private view returns (uint256) { /// @audit Missing: '@return' 831 * of cash tokens for in a given epoch 832 */ 833 function getBurnedQuantity( 834 uint256 epoch, 835 address user 836: ) external view returns (uint256) { /// @audit Missing: '@return' 947 * 3) value - eth value to call target with 948 */ 949 function multiexcall( 950 ExCallData[] calldata exCallData 951 ) 952 external 953 payable 954 override 955 nonReentrant 956 onlyRole(MANAGER_ADMIN) 957 whenPaused 958: returns (bytes[] memory results)
File: contracts/cash/factory/CashFactory.sol /// @audit Missing: '@return' 121 * 3) value - eth value to call target with 122 */ 123 function multiexcall( 124 ExCallData[] calldata exCallData 125: ) external payable override onlyGuardian returns (bytes[] memory results) {
File: contracts/cash/factory/CashKYCSenderFactory.sol /// @audit Missing: '@param registry' /// @audit Missing: '@param kycRequirementGroup' 57 /** 58 * @dev This function will deploy an upgradable instance of CashKYCSender 59 * 60 * @param name The name of the token we want to deploy. 61 * @param ticker The ticker for the token we want to deploy. 62 * 63 * @return address The address of the proxy contract. 64 * @return address The address of the proxyAdmin contract. 65 * @return address The address of the implementation contract. 66 * 67 * @notice 1) Will automatically revoke all deployer roles granted to 68 * address(this). 69 * 2) Will grant DEFAULT_ADMIN & PAUSER_ROLE(S) to `guardian` 70 * address specified in constructor. 71 * 3) Will transfer ownership of the proxyAdmin to guardian 72 * address. 73 * 74 */ 75 function deployCashKYCSender( 76 string calldata name, 77 string calldata ticker, 78 address registry, 79 uint256 kycRequirementGroup 80: ) external onlyGuardian returns (address, address, address) { /// @audit Missing: '@return' 131 * 3) value - eth value to call target with 132 */ 133 function multiexcall( 134 ExCallData[] calldata exCallData 135: ) external payable override onlyGuardian returns (bytes[] memory results) {
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol /// @audit Missing: '@param registry' /// @audit Missing: '@param kycRequirementGroup' 57 /** 58 * @dev This function will deploy an upgradable instance of CashKYCSenderReceiver 59 * 60 * @param name The name of the token we want to deploy. 61 * @param ticker The ticker for the token we want to deploy. 62 * 63 * @return address The address of the proxy contract. 64 * @return address The address of the proxyAdmin contract. 65 * @return address The address of the implementation contract. 66 * 67 * @notice 1) Will automatically revoke all deployer roles granted to 68 * address(this). 69 * 2) Will grant DEFAULT_ADMIN & PAUSER_ROLE(S) to `guardian` 70 * address specified in constructor. 71 * 3) Will transfer ownership of the proxyAdmin to guardian 72 * address. 73 * 74 */ 75 function deployCashKYCSenderReceiver( 76 string calldata name, 77 string calldata ticker, 78 address registry, 79 uint256 kycRequirementGroup 80: ) external onlyGuardian returns (address, address, address) { /// @audit Missing: '@return' 131 * 3) value - eth value to call target with 132 */ 133 function multiexcall( 134 ExCallData[] calldata exCallData 135: ) external payable override onlyGuardian returns (bytes[] memory results) {
File: contracts/cash/kyc/KYCRegistryClient.sol /// @audit Missing: '@return' 63 * @param account The address to check 64 */ 65: function _getKYCStatus(address account) internal view returns (bool) {
File: contracts/cash/kyc/KYCRegistry.sol /// @audit Missing: '@return' 126 * @param account Addresses to check KYC status for 127 */ 128 function getKYCStatus( 129 uint256 kycRequirementGroup, 130 address account 131: ) external view override returns (bool) {
File: contracts/lending/OndoPriceOracle.sol /// @audit Missing: '@return' 59 * set within `fTokenToCToken` and piggy back on an external price oracle. 60 */ 61 function getUnderlyingPrice( 62 address fToken 63: ) external view override returns (uint256) {
File: contracts/lending/OndoPriceOracleV2.sol /// @audit Missing: '@return' 90 * @dev Only supports oracle prices denominated in USD 91 */ 92 function getUnderlyingPrice( 93 address fToken 94: ) external view override returns (uint256) { /// @audit Missing: '@param value' 125 /** 126 * @notice Sets the price cap for the provided fToken's underlying asset 127 * 128 * @param fToken fToken contract address 129 */ 130 function setPriceCap( 131 address fToken, 132 uint256 value 133: ) external override onlyOwner { /// @audit Missing: '@return' 275 * @dev This function is public for observability purposes only. 276 */ 277 function getChainlinkOraclePrice( 278 address fToken 279: ) public view returns (uint256) {
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
There are 3 instances of this issue:
File: contracts/cash/CashManager.sol 259: mintRequestsPerEpoch[epochToClaim][user] = 0; 754: redemptionInfoPerEpoch[epochToService].addressToBurnAmt[redeemer] = 0; 790: redemptionInfoPerEpoch[epochToService].addressToBurnAmt[refundee] = 0;
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.
There is 1 instance of this issue:
File: Various Files
Large code bases, or code with lots of inline-assembly, complicated math, or complicated interactions between multiple contracts, should implement fuzzing tests. Fuzzers such as Echidna require the test writer to come up with invariants which should not be violated under any circumstances, and the fuzzer tests various inputs and function calls to ensure that the invariants always hold. Even code with 100% code coverage can still have bugs due to the order of the operations a user performs, and fuzzers, with properly and extensively-written invariants, can close this testing gap significantly.
There is 1 instance of this issue:
File: Various Files
According to the Solidity style guide, functions should be laid out in the following order :constructor(), receive(), fallback(), external, public, internal, private, but the cases below do not follow this pattern
There are 8 instances of this issue:
File: contracts/cash/CashManager.sol /// @audit _scaleUp() came earlier 526: function pause() external onlyRole(PAUSER_ADMIN) { /// @audit transitionEpoch() came earlier 596: function setMintLimit(uint256 _mintLimit) external onlyRole(MANAGER_ADMIN) { /// @audit _checkAndUpdateRedeemLimit() came earlier 662 function requestRedemption( 663 uint256 amountCashToRedeem 664 ) 665 external 666 override 667 updateEpoch 668 nonReentrant 669 whenNotPaused 670: checkKYC(msg.sender) /// @audit _processRefund() came earlier 803 function setAssetSender( 804 address newAssetSender 805: ) external onlyRole(MANAGER_ADMIN) { /// @audit _checkAddressesKYC() came earlier 949 function multiexcall( 950 ExCallData[] calldata exCallData 951 ) 952 external 953 payable 954 override 955 nonReentrant 956 onlyRole(MANAGER_ADMIN) 957 whenPaused 958: returns (bytes[] memory results)
File: contracts/lending/OndoPriceOracleV2.sol /// @audit _setFTokenToCToken() came earlier 233 function setFTokenToChainlinkOracle( 234 address fToken, 235 address newChainlinkOracle 236: ) external override onlyOwner { /// @audit _setFTokenToChainlinkOracle() came earlier 277 function getChainlinkOraclePrice( 278 address fToken 279: ) public view returns (uint256) { /// @audit getChainlinkOraclePrice() came earlier 309 function setMaxChainlinkOracleTimeDelay( 310 uint256 _maxChainlinkOracleTimeDelay 311: ) external override onlyOwner {
The style guide says that, within a contract, the ordering should be 1) Type declarations, 2) State variables, 3) Events, 4) Modifiers, and 5) Functions, but the contract(s) below do not follow this ordering
There are 14 instances of this issue:
File: contracts/cash/CashManager.sol /// @audit function setEpochDuration came earlier 557 modifier updateEpoch() { 558 transitionEpoch(); 559 _; 560: } /// @audit function setPendingRedemptionBalance came earlier 885 modifier checkKYC(address account) { 886 _checkKYC(account); 887 _; 888: }
File: contracts/cash/factory/CashFactory.sol /// @audit function multiexcall came earlier 143 event CashDeployed( 144 address proxy, 145 address proxyAdmin, 146 address implementation, 147 string name, 148 string ticker 149: );
File: contracts/cash/factory/CashKYCSenderFactory.sol /// @audit function multiexcall came earlier 153 event CashKYCSenderDeployed( 154 address proxy, 155 address proxyAdmin, 156 address implementation, 157 string name, 158 string ticker, 159 address registry 160: );
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol /// @audit function multiexcall came earlier 153 event CashKYCSenderReceiverDeployed( 154 address proxy, 155 address proxyAdmin, 156 address implementation, 157 string name, 158 string ticker, 159 address registry 160: );
File: contracts/cash/interfaces/ICashManager.sol /// @audit function completeRedemptions came earlier 76: event FeeRecipientSet(address oldFeeRecipient, address newFeeRecipient);
File: contracts/cash/interfaces/IKYCRegistryClient.sol /// @audit function setKYCRegistry came earlier 48: event KYCRegistrySet(address oldRegistry, address newRegistry);
File: contracts/cash/kyc/KYCRegistry.sol /// @audit function removeKYCAddresses came earlier 195 event RoleAssignedToKYCGroup( 196 uint256 indexed kycRequirementGroup, 197 bytes32 indexed role 198: );
File: contracts/lending/IOndoPriceOracle.sol /// @audit function setOracle came earlier 41 event FTokenToCTokenSet( 42 address indexed fToken, 43 address oldCToken, 44 address newCToken 45: );
File: contracts/lending/IOndoPriceOracleV2.sol /// @audit function setOracle came earlier 41 event FTokenToCTokenSet( 42 address indexed fToken, 43 address oldCToken, 44 address newCToken 45: ); /// @audit event CTokenOracleSet came earlier 71 enum OracleType { 72 UNINITIALIZED, 73 MANUAL, 74 COMPOUND, 75 CHAINLINK 76: } /// @audit function setFTokenToOracleType came earlier 101 event PriceCapSet( 102 address indexed fToken, 103 uint256 oldPriceCap, 104 uint256 newPriceCap 105: );
File: contracts/lending/OndoPriceOracle.sol /// @audit function underlying came earlier 41 CTokenOracle public cTokenOracle = 42: CTokenOracle(0x65c816077C29b557BEE980ae3cC2dCE80204A0C5);
File: contracts/lending/OndoPriceOracleV2.sol /// @audit function decimals came earlier 51 CTokenOracle public cTokenOracle = 52: CTokenOracle(0x65c816077C29b557BEE980ae3cC2dCE80204A0C5);
I prefix in the contract nameThere are 6 instances of this issue:
File: contracts/lending/IOndoPriceOracle.sol 18: interface PriceOracle {
File: contracts/lending/IOndoPriceOracleV2.sol 18: interface PriceOracle {
File: contracts/lending/OndoPriceOracle.sol 21: interface CTokenOracle { 27: interface CTokenLike {
File: contracts/lending/OndoPriceOracleV2.sol 22: interface CTokenOracle { 28: interface CTokenLike {
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness
| Issue | Instances | |
|---|---|---|
| [Lā01] | Missing checks for address(0x0) when assigning values to address state variables | 6 |
Total: 6 instances over 1 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | constants should be defined rather than using magic numbers | 1 |
| [Nā02] | Event is missing indexed fields | 38 |
Total: 39 instances over 2 issues
address(0x0) when assigning values to address state variablesThere are 6 instances of this issue:
File: contracts/cash/CashManager.sol /// @audit (valid but excluded finding) 456: feeRecipient = _feeRecipient; /// @audit (valid but excluded finding) 469: assetRecipient = _assetRecipient; /// @audit (valid but excluded finding) 807: assetSender = newAssetSender;
File: contracts/cash/factory/CashFactory.sol /// @audit (valid but excluded finding) 54: guardian = _guardian;
File: contracts/cash/factory/CashKYCSenderFactory.sol /// @audit (valid but excluded finding) 54: guardian = _guardian;
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol /// @audit (valid but excluded finding) 54: guardian = _guardian;
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There is 1 instance of this issue:
File: contracts/lending/OndoPriceOracleV2.sol /// @audit 36 - (valid but excluded finding) 261: (36 -
indexed fieldsIndex event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
There are 38 instances of this issue:
File: contracts/cash/factory/CashFactory.sol /// @audit (valid but excluded finding) 143 event CashDeployed( 144 address proxy, 145 address proxyAdmin, 146 address implementation, 147 string name, 148 string ticker 149: );
File: contracts/cash/factory/CashKYCSenderFactory.sol /// @audit (valid but excluded finding) 153 event CashKYCSenderDeployed( 154 address proxy, 155 address proxyAdmin, 156 address implementation, 157 string name, 158 string ticker, 159 address registry 160: );
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol /// @audit (valid but excluded finding) 153 event CashKYCSenderReceiverDeployed( 154 address proxy, 155 address proxyAdmin, 156 address implementation, 157 string name, 158 string ticker, 159 address registry 160: );
File: contracts/cash/interfaces/ICashManager.sol /// @audit (valid but excluded finding) 76: event FeeRecipientSet(address oldFeeRecipient, address newFeeRecipient); /// @audit (valid but excluded finding) 84: event AssetRecipientSet(address oldAssetRecipient, address newAssetRecipient); /// @audit (valid but excluded finding) 92: event AssetSenderSet(address oldAssetSender, address newAssetSender); /// @audit (valid but excluded finding) 102: event MinimumDepositAmountSet(uint256 oldMinimum, uint256 newMinimum); /// @audit (valid but excluded finding) 111: event MinimumRedeemAmountSet(uint256 oldRedeemMin, uint256 newRedeemMin); /// @audit (valid but excluded finding) 121: event MintFeeSet(uint256 oldFee, uint256 newFee); /// @audit (valid but excluded finding) 133 event MintExchangeRateSet( 134 uint256 indexed epoch, 135 uint256 oldRate, 136 uint256 newRate 137: ); /// @audit (valid but excluded finding) 151 event MintExchangeRateOverridden( 152 uint256 indexed epoch, 153 uint256 oldRate, 154 uint256 newRate, 155 uint256 lastSetMintExchangeRate 156: ); /// @audit (valid but excluded finding) 164: event ExchangeRateDeltaLimitSet(uint256 oldLimit, uint256 newLimit); /// @audit (valid but excluded finding) 175 event MintExchangeRateCheckFailed( 176 uint256 indexed epoch, 177 uint256 lastEpochRate, 178 uint256 newRate 179: ); /// @audit (valid but excluded finding) 189: event MintLimitSet(uint256 oldLimit, uint256 newLimit); /// @audit (valid but excluded finding) 199: event RedeemLimitSet(uint256 oldLimit, uint256 newLimit); /// @audit (valid but excluded finding) 209: event EpochDurationSet(uint256 oldDuration, uint256 newDuration); /// @audit (valid but excluded finding) 218 event RedemptionRequested( 219 address indexed user, 220 uint256 cashAmountIn, 221 uint256 indexed epoch 222: ); /// @audit (valid but excluded finding) 233 event MintRequested( 234 address indexed user, 235 uint256 indexed epoch, 236 uint256 collateralAmountDeposited, 237 uint256 depositAmountAfterFee, 238 uint256 feeAmount 239: ); /// @audit (valid but excluded finding) 250 event RedemptionCompleted( 251 address indexed user, 252 uint256 cashAmountRequested, 253 uint256 collateralAmountReturned, 254 uint256 indexed epoch 255: ); /// @audit (valid but excluded finding) 268 event MintCompleted( 269 address indexed user, 270 uint256 cashAmountOut, 271 uint256 collateralAmountDeposited, 272 uint256 exchangeRate, 273 uint256 indexed epochClaimedFrom 274: ); /// @audit (valid but excluded finding) 284 event RedemptionFeesCollected( 285 address indexed beneficiary, 286 uint256 collateralAmountOut, 287 uint256 indexed epoch 288: ); /// @audit (valid but excluded finding) 297 event RefundIssued( 298 address indexed user, 299 uint256 cashAmountOut, 300 uint256 indexed epoch 301: ); /// @audit (valid but excluded finding) 311 event PendingMintBalanceSet( 312 address indexed user, 313 uint256 indexed epoch, 314 uint256 oldBalance, 315 uint256 newBalance 316: ); /// @audit (valid but excluded finding) 327 event PendingRedemptionBalanceSet( 328 address indexed user, 329 uint256 indexed epoch, 330 uint256 balance, 331 uint256 totalBurned 332: );
File: contracts/cash/interfaces/IKYCRegistryClient.sol /// @audit (valid but excluded finding) 48: event KYCRegistrySet(address oldRegistry, address newRegistry); /// @audit (valid but excluded finding) 56 event KYCRequirementGroupSet( 57 uint256 oldRequirementGroup, 58 uint256 newRequirementGroup 59: );
File: contracts/cash/kyc/KYCRegistry.sol /// @audit (valid but excluded finding) 207 event KYCAddressesAdded( 208 address indexed sender, 209 uint256 indexed kycRequirementGroup, 210 address[] addresses 211: ); /// @audit (valid but excluded finding) 238 event KYCAddressesRemoved( 239 address indexed sender, 240 uint256 indexed kycRequirementGroup, 241 address[] addresses 242: );
File: contracts/lending/IOndoPriceOracle.sol /// @audit (valid but excluded finding) 41 event FTokenToCTokenSet( 42 address indexed fToken, 43 address oldCToken, 44 address newCToken 45: ); /// @audit (valid but excluded finding) 54 event UnderlyingPriceSet( 55 address indexed fToken, 56 uint256 oldPrice, 57 uint256 newPrice 58: ); /// @audit (valid but excluded finding) 66: event CTokenOracleSet(address oldOracle, address newOracle);
File: contracts/lending/IOndoPriceOracleV2.sol /// @audit (valid but excluded finding) 41 event FTokenToCTokenSet( 42 address indexed fToken, 43 address oldCToken, 44 address newCToken 45: ); /// @audit (valid but excluded finding) 54 event UnderlyingPriceSet( 55 address indexed fToken, 56 uint256 oldPrice, 57 uint256 newPrice 58: ); /// @audit (valid but excluded finding) 66: event CTokenOracleSet(address oldOracle, address newOracle); /// @audit (valid but excluded finding) 101 event PriceCapSet( 102 address indexed fToken, 103 uint256 oldPriceCap, 104 uint256 newPriceCap 105: ); /// @audit (valid but excluded finding) 114 event ChainlinkOracleSet( 115 address indexed fToken, 116 address oldOracle, 117 address newOracle 118: ); /// @audit (valid but excluded finding) 126 event MaxChainlinkOracleTimeDelaySet( 127 uint256 oldMaxTimeDelay, 128 uint256 newMaxTimeDelay 129: ); /// @audit (valid but excluded finding) 137: event FTokenToOracleTypeSet(address indexed fToken, OracleType oracleType);
#0 - c4-judge
2023-01-23T11:15:35Z
trust1995 marked the issue as grade-a
#1 - trust1995
2023-01-23T15:48:01Z
Great report, top 3 QA.
#2 - ypatil12
2023-01-24T15:50:23Z
Amazing report
š Selected for report: c3phas
Also found by: 0x1f8b, 0xSmartContract, Aymen0909, Bnke0x0, Diana, IllIllI, RaymondFam, Rolezn, Sathish9098, SleepingBugs, Viktor_Cortess, adriro, arialblack14, chaduke, cryptostellar5, cygaar, descharre, dharma09, eyexploit, halden, pavankv, saneryee, tsvetanovv
32.3616 USDC - $32.36
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [Gā01] | Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate | 4 | - |
| [Gā02] | Avoid contract existence checks by using low level calls | 2 | 200 |
| [Gā03] | State variables should be cached in stack variables rather than re-reading them from storage | 15 | 1455 |
| [Gā04] | Multiple accesses of a mapping/array should use a local variable cache | 8 | 336 |
| [Gā05] | <x> += <y> costs more gas than <x> = <x> + <y> for state variables | 3 | 339 |
| [Gā06] | internal functions only called once can be inlined to save gas | 4 | 80 |
| [Gā07] | Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement | 4 | 340 |
| [Gā08] | ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops | 9 | 540 |
| [Gā09] | require()/revert() strings longer than 32 bytes cost extra gas | 12 | - |
| [Gā10] | Optimize names to save gas | 8 | 176 |
| [Gā11] | Use a more recent version of solidity | 2 | - |
| [Gā12] | Splitting require() statements that use && saves gas | 1 | 3 |
| [Gā13] | Using private rather than public for constants, saves gas | 1 | - |
| [Gā14] | Use custom errors rather than revert()/require() strings to save gas | 16 | - |
| [Gā15] | Functions guaranteed to revert when called by normal users can be marked payable | 34 | 714 |
| [Gā16] | Don't use _msgSender() if not supporting EIP-2771 | 3 | 48 |
Total: 126 instances over 16 issues with 4231 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriateSaves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.
There are 4 instances of this issue:
File: contracts/cash/CashManager.sol 79 mapping(uint256 => RedemptionRequests) public redemptionInfoPerEpoch; 80 81 // Mapping used for getting the exchange rate during a given epoch 82 mapping(uint256 => uint256) public epochToExchangeRate; 83 84 // Nested mapping containing mint requests for an epoch 85 // { <epoch> : {<user> : <collateralAmount> } 86: mapping(uint256 => mapping(address => uint256)) public mintRequestsPerEpoch;
File: contracts/lending/OndoPriceOracle.sol 45 mapping(address => uint256) public fTokenToUnderlyingPrice; 46 47 /// @notice fToken to cToken associations for piggy backing off 48 /// of cToken oracles 49: mapping(address => address) public fTokenToCToken;
File: contracts/lending/OndoPriceOracleV2.sol 55 mapping(address => OracleType) public fTokenToOracleType; 56 57 /// @notice Contract storage for fToken's underlying asset prices 58 mapping(address => uint256) public fTokenToUnderlyingPrice; 59 60 /// @notice fToken to cToken associations for piggy backing off 61 /// of Compound's Oracle 62: mapping(address => address) public fTokenToCToken; 70 mapping(address => ChainlinkOracleInfo) public fTokenToChainlinkOracle; 71 72 /// @notice Price cap for the underlying asset of an fToken. Optional. 73: mapping(address => uint256) public fTokenToUnderlyingPriceCap;
Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external function calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value. Similar behavior can be achieved in earlier versions by using low-level calls, since low level calls never check for contract existence
There are 2 instances of this issue:
File: contracts/lending/OndoPriceOracle.sol /// @audit underlying() /// @audit underlying() 121: CTokenLike(fToken).underlying() == CTokenLike(cToken).underlying(),
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There are 15 instances of this issue:
File: contracts/cash/CashManager.sol /// @audit feeRecipient on line 725 726: emit RedemptionFeesCollected(feeRecipient, fees, epochToService); /// @audit minimumRedeemAmount on line 821 822: emit MinimumRedeemAmountSet(oldRedeemMin, minimumRedeemAmount); /// @audit currentEpoch on line 221 225: currentEpoch, /// @audit currentEpoch on line 678 681: redemptionInfoPerEpoch[currentEpoch].totalBurned += amountCashToRedeem; /// @audit currentEpoch on line 681 685: emit RedemptionRequested(msg.sender, amountCashToRedeem, currentEpoch); /// @audit epochDuration on line 172 176: (block.timestamp % epochDuration); /// @audit epochDuration on line 578 585: (block.timestamp % epochDuration); /// @audit lastSetMintExchangeRate on line 296 297: rateDifference = exchangeRate - lastSetMintExchangeRate; /// @audit lastSetMintExchangeRate on line 297 298: } else if (exchangeRate < lastSetMintExchangeRate) { /// @audit lastSetMintExchangeRate on line 298 299: rateDifference = lastSetMintExchangeRate - exchangeRate; /// @audit lastSetMintExchangeRate on line 299 302: uint256 maxDifferenceThisEpoch = (lastSetMintExchangeRate * /// @audit lastSetMintExchangeRate on line 302 310: lastSetMintExchangeRate, /// @audit lastSetMintExchangeRate on line 310 314: uint256 oldExchangeRate = lastSetMintExchangeRate; /// @audit lastSetMintExchangeRate on line 377 383: lastSetMintExchangeRate /// @audit redemptionInfoPerEpoch[epoch].totalBurned on line 867 874: redemptionInfoPerEpoch[epoch].totalBurned
The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory/calldata
There are 8 instances of this issue:
File: contracts/cash/CashManager.sol /// @audit redemptionInfoPerEpoch[currentEpoch] on line 678 681: redemptionInfoPerEpoch[currentEpoch].totalBurned += amountCashToRedeem; /// @audit redemptionInfoPerEpoch[epochToService] on line 752 754: redemptionInfoPerEpoch[epochToService].addressToBurnAmt[redeemer] = 0; /// @audit redemptionInfoPerEpoch[epochToService] on line 788 790: redemptionInfoPerEpoch[epochToService].addressToBurnAmt[refundee] = 0; /// @audit redemptionInfoPerEpoch[epoch] on line 859 865: redemptionInfoPerEpoch[epoch].totalBurned -= previousBalance - balance; /// @audit redemptionInfoPerEpoch[epoch] on line 865 867: redemptionInfoPerEpoch[epoch].totalBurned += balance - previousBalance; /// @audit redemptionInfoPerEpoch[epoch] on line 867 869: redemptionInfoPerEpoch[epoch].addressToBurnAmt[user] = balance; /// @audit redemptionInfoPerEpoch[epoch] on line 869 874: redemptionInfoPerEpoch[epoch].totalBurned
File: contracts/lending/OndoPriceOracleV2.sol /// @audit fTokenToChainlinkOracle[fToken] on line 260 264: fTokenToChainlinkOracle[fToken].oracle = AggregatorV3Interface(
<x> += <y> costs more gas than <x> = <x> + <y> for state variablesUsing the addition operator instead of plus-equals saves 113 gas
There are 3 instances of this issue:
File: contracts/cash/CashManager.sol 582: currentEpoch += epochDifference; 630: currentMintAmount += collateralAmountIn; 649: currentRedeemAmount += amount;
internal functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 4 instances of this issue:
File: contracts/lending/OndoPriceOracle.sol 119: function _setFTokenToCToken(address fToken, address cToken) internal {
File: contracts/lending/OndoPriceOracleV2.sol 210: function _setFTokenToCToken(address fToken, address cToken) internal { 251 function _setFTokenToChainlinkOracle( 252 address fToken, 253: address chainlinkOracle 324: function _min(uint256 a, uint256 b) internal pure returns (uint256) {
unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statementrequire(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }
There are 4 instances of this issue:
File: contracts/cash/CashManager.sol /// @audit if-condition on line 296 297: rateDifference = exchangeRate - lastSetMintExchangeRate; /// @audit if-condition on line 298 299: rateDifference = lastSetMintExchangeRate - exchangeRate; /// @audit if-condition on line 864 865: redemptionInfoPerEpoch[epoch].totalBurned -= previousBalance - balance; /// @audit if-condition on line 866 867: redemptionInfoPerEpoch[epoch].totalBurned += balance - previousBalance;
++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loopsThe unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop
There are 9 instances of this issue:
File: contracts/cash/CashManager.sol 750: for (uint256 i = 0; i < size; ++i) { 786: for (uint256 i = 0; i < size; ++i) { 933: for (uint256 i = 0; i < size; ++i) { 961: for (uint256 i = 0; i < exCallData.length; ++i) {
File: contracts/cash/factory/CashFactory.sol 127: for (uint256 i = 0; i < exCallData.length; ++i) {
File: contracts/cash/factory/CashKYCSenderFactory.sol 137: for (uint256 i = 0; i < exCallData.length; ++i) {
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol 137: for (uint256 i = 0; i < exCallData.length; ++i) {
File: contracts/cash/kyc/KYCRegistry.sol 163: for (uint256 i = 0; i < length; i++) { 180: for (uint256 i = 0; i < length; i++) {
require()/revert() strings longer than 32 bytes cost extra gasEach extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas
There are 12 instances of this issue:
File: contracts/cash/factory/CashKYCSenderFactory.sol 163 require( 164 msg.sender == guardian, 165 "CashKYCSenderFactory: You are not the Guardian" 166: );
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol 163 require( 164 msg.sender == guardian, 165 "CashKYCSenderReceiverFactory: You are not the Guardian" 166: );
File: contracts/cash/kyc/KYCRegistry.sol 88 require( 89 !kycState[kycRequirementGroup][user], 90 "KYCRegistry: user already verified" 91: );
File: contracts/cash/token/CashKYCSenderReceiver.sol 63 require( 64 _getKYCStatus(_msgSender()), 65 "CashKYCSenderReceiver: must be KYC'd to initiate transfer" 66: ); 70 require( 71 _getKYCStatus(from), 72 "CashKYCSenderReceiver: `from` address must be KYC'd to send tokens" 73: ); 78 require( 79 _getKYCStatus(to), 80 "CashKYCSenderReceiver: `to` address must be KYC'd to receive tokens" 81: );
File: contracts/cash/token/CashKYCSender.sol 63 require( 64 _getKYCStatus(_msgSender()), 65 "CashKYCSender: must be KYC'd to initiate transfer" 66: ); 70 require( 71 _getKYCStatus(from), 72 "CashKYCSender: `from` address must be KYC'd to send tokens" 73: );
File: contracts/cash/token/Cash.sol 36 require( 37 hasRole(TRANSFER_ROLE, _msgSender()), 38 "Cash: must have TRANSFER_ROLE to transfer" 39: );
File: contracts/lending/OndoPriceOracle.sol 120 require( 121 CTokenLike(fToken).underlying() == CTokenLike(cToken).underlying(), 122 "cToken and fToken must have the same underlying asset if cToken nonzero" 123: );
File: contracts/lending/OndoPriceOracleV2.sol 215 require( 216 CTokenLike(fToken).underlying() == CTokenLike(cToken).underlying(), 217 "cToken and fToken must have the same underlying asset" 218: ); 280 require( 281 fTokenToOracleType[fToken] == OracleType.CHAINLINK, 282 "fToken is not configured for Chainlink oracle" 283: );
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted
There are 8 instances of this issue:
File: contracts/cash/CashManager.sol /// @audit setPendingMintBalance(), setEpochDuration(), transitionEpoch(), setMintLimit(), setRedeemLimit(), setAssetSender(), setRedeemMinimum(), getBurnedQuantity(), setPendingRedemptionBalance() 28: contract CashManager is
File: contracts/cash/factory/CashFactory.sol /// @audit deployCash() 43: contract CashFactory is IMulticall {
File: contracts/cash/interfaces/ICashManager.sol /// @audit requestMint(), claimMint(), overrideExchangeRate(), setAssetRecipient(), setFeeRecipient(), setAssetSender(), setMinimumDepositAmount(), setMintFee(), setMintExchangeRate(), setMintExchangeRateDeltaLimit(), requestRedemption(), completeRedemptions() 17: interface ICashManager {
File: contracts/cash/interfaces/IKYCRegistryClient.sol /// @audit kycRequirementGroup(), kycRegistry(), setKYCRequirementGroup(), setKYCRegistry() 25: interface IKYCRegistryClient {
File: contracts/cash/kyc/KYCRegistry.sol /// @audit addKYCAddressViaSignature(), assignRoletoKYCGroup(), addKYCAddresses(), removeKYCAddresses() 30: contract KYCRegistry is AccessControlEnumerable, IKYCRegistry, EIP712 {
File: contracts/lending/IOndoPriceOracle.sol /// @audit setPrice(), setFTokenToCToken(), setOracle() 27: interface IOndoPriceOracle is PriceOracle {
File: contracts/lending/IOndoPriceOracleV2.sol /// @audit setPrice(), setFTokenToCToken(), setOracle() 27: interface IOndoPriceOracle is PriceOracle { /// @audit setPriceCap(), setFTokenToChainlinkOracle(), setMaxChainlinkOracleTimeDelay(), setFTokenToOracleType() 69: interface IOndoPriceOracleV2 is IOndoPriceOracle {
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
There are 2 instances of this issue:
File: contracts/lending/IOndoPriceOracle.sol 15: pragma solidity 0.6.12;
File: contracts/lending/OndoPriceOracle.sol 15: pragma solidity 0.6.12;
require() statements that use && saves gasSee this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper by 3 gas
There is 1 instance of this issue:
File: contracts/lending/OndoPriceOracleV2.sol 292 require( 293 (answeredInRound >= roundId) && 294 (updatedAt >= block.timestamp - maxChainlinkOracleTimeDelay), 295 "Chainlink oracle price is stale" 296: );
private rather than public for constants, saves gasIf needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table
There is 1 instance of this issue:
File: contracts/cash/CashManager.sol 93: uint256 public immutable decimalsMultiplier;
revert()/require() strings to save gasCustom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas
There are 16 instances of this issue:
File: contracts/cash/factory/CashKYCSenderFactory.sol 163 require( 164 msg.sender == guardian, 165 "CashKYCSenderFactory: You are not the Guardian" 166: );
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol 163 require( 164 msg.sender == guardian, 165 "CashKYCSenderReceiverFactory: You are not the Guardian" 166: );
File: contracts/cash/kyc/KYCRegistry.sol 88 require( 89 !kycState[kycRequirementGroup][user], 90 "KYCRegistry: user already verified" 91: );
File: contracts/cash/token/CashKYCSenderReceiver.sol 63 require( 64 _getKYCStatus(_msgSender()), 65 "CashKYCSenderReceiver: must be KYC'd to initiate transfer" 66: ); 70 require( 71 _getKYCStatus(from), 72 "CashKYCSenderReceiver: `from` address must be KYC'd to send tokens" 73: ); 78 require( 79 _getKYCStatus(to), 80 "CashKYCSenderReceiver: `to` address must be KYC'd to receive tokens" 81: );
File: contracts/cash/token/CashKYCSender.sol 63 require( 64 _getKYCStatus(_msgSender()), 65 "CashKYCSender: must be KYC'd to initiate transfer" 66: ); 70 require( 71 _getKYCStatus(from), 72 "CashKYCSender: `from` address must be KYC'd to send tokens" 73: );
File: contracts/cash/token/Cash.sol 36 require( 37 hasRole(TRANSFER_ROLE, _msgSender()), 38 "Cash: must have TRANSFER_ROLE to transfer" 39: );
File: contracts/lending/OndoPriceOracle.sol 120 require( 121 CTokenLike(fToken).underlying() == CTokenLike(cToken).underlying(), 122 "cToken and fToken must have the same underlying asset if cToken nonzero" 123: );
File: contracts/lending/OndoPriceOracleV2.sol 164 require( 165 fTokenToOracleType[fToken] == OracleType.MANUAL, 166 "OracleType must be Manual" 167: ); 211 require( 212 fTokenToOracleType[fToken] == OracleType.COMPOUND, 213 "OracleType must be Compound" 214: ); 215 require( 216 CTokenLike(fToken).underlying() == CTokenLike(cToken).underlying(), 217 "cToken and fToken must have the same underlying asset" 218: ); 255 require( 256 fTokenToOracleType[fToken] == OracleType.CHAINLINK, 257 "OracleType must be Chainlink" 258: ); 280 require( 281 fTokenToOracleType[fToken] == OracleType.CHAINLINK, 282 "fToken is not configured for Chainlink oracle" 283: ); 292 require( 293 (answeredInRound >= roundId) && 294 (updatedAt >= block.timestamp - maxChainlinkOracleTimeDelay), 295 "Chainlink oracle price is stale" 296: );
payableIf a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
There are 34 instances of this issue:
File: contracts/cash/CashManager.sol 281 function setMintExchangeRate( 282 uint256 exchangeRate, 283 uint256 epochToSet 284: ) external override updateEpoch onlyRole(SETTER_ADMIN) { 336 function setPendingMintBalance( 337 address user, 338 uint256 epoch, 339 uint256 oldBalance, 340 uint256 newBalance 341: ) external updateEpoch onlyRole(MANAGER_ADMIN) { 366 function overrideExchangeRate( 367 uint256 correctExchangeRate, 368 uint256 epochToSet, 369 uint256 _lastSetMintExchangeRate 370: ) external override updateEpoch onlyRole(MANAGER_ADMIN) { 392 function setMintExchangeRateDeltaLimit( 393 uint256 _exchangeRateDeltaLimit 394: ) external override onlyRole(MANAGER_ADMIN) { 410 function setMintFee( 411 uint256 _mintFee 412: ) external override onlyRole(MANAGER_ADMIN) { 433 function setMinimumDepositAmount( 434 uint256 _minimumDepositAmount 435: ) external override onlyRole(MANAGER_ADMIN) { 452 function setFeeRecipient( 453 address _feeRecipient 454: ) external override onlyRole(MANAGER_ADMIN) { 465 function setAssetRecipient( 466 address _assetRecipient 467: ) external override onlyRole(MANAGER_ADMIN) { 546 function setEpochDuration( 547 uint256 _epochDuration 548: ) external onlyRole(MANAGER_ADMIN) { 609 function setRedeemLimit( 610 uint256 _redeemLimit 611: ) external onlyRole(MANAGER_ADMIN) { 707 function completeRedemptions( 708 address[] calldata redeemers, 709 address[] calldata refundees, 710 uint256 collateralAmountToDist, 711 uint256 epochToService, 712 uint256 fees 713: ) external override updateEpoch onlyRole(MANAGER_ADMIN) { 803 function setAssetSender( 804 address newAssetSender 805: ) external onlyRole(MANAGER_ADMIN) { 817 function setRedeemMinimum( 818 uint256 newRedeemMinimum 819: ) external onlyRole(MANAGER_ADMIN) { 851 function setPendingRedemptionBalance( 852 address user, 853 uint256 epoch, 854 uint256 balance 855: ) external updateEpoch onlyRole(MANAGER_ADMIN) { 896 function setKYCRequirementGroup( 897 uint256 _kycRequirementGroup 898: ) external override onlyRole(MANAGER_ADMIN) { 907 function setKYCRegistry( 908 address _kycRegistry 909: ) external override onlyRole(MANAGER_ADMIN) {
File: contracts/cash/factory/CashFactory.sol 75 function deployCash( 76 string calldata name, 77 string calldata ticker 78: ) external onlyGuardian returns (address, address, address) {
File: contracts/cash/factory/CashKYCSenderFactory.sol 75 function deployCashKYCSender( 76 string calldata name, 77 string calldata ticker, 78 address registry, 79 uint256 kycRequirementGroup 80: ) external onlyGuardian returns (address, address, address) {
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol 75 function deployCashKYCSenderReceiver( 76 string calldata name, 77 string calldata ticker, 78 address registry, 79 uint256 kycRequirementGroup 80: ) external onlyGuardian returns (address, address, address) {
File: contracts/cash/kyc/KYCRegistryClientInitializable.sol 44 function __KYCRegistryClientInitializable_init( 45 address _kycRegistry, 46 uint256 _kycRequirementGroup 47: ) internal onlyInitializing { 58 function __KYCRegistryClientInitializable_init_unchained( 59 address _kycRegistry, 60 uint256 _kycRequirementGroup 61: ) internal onlyInitializing {
File: contracts/cash/kyc/KYCRegistry.sol 144 function assignRoletoKYCGroup( 145 uint256 kycRequirementGroup, 146 bytes32 role 147: ) external onlyRole(REGISTRY_ADMIN) { 158 function addKYCAddresses( 159 uint256 kycRequirementGroup, 160 address[] calldata addresses 161: ) external onlyRole(kycGroupRoles[kycRequirementGroup]) { 175 function removeKYCAddresses( 176 uint256 kycRequirementGroup, 177 address[] calldata addresses 178: ) external onlyRole(kycGroupRoles[kycRequirementGroup]) {
File: contracts/cash/token/CashKYCSenderReceiver.sol 34 function setKYCRequirementGroup( 35 uint256 group 36: ) external override onlyRole(KYC_CONFIGURER_ROLE) { 40 function setKYCRegistry( 41 address registry 42: ) external override onlyRole(KYC_CONFIGURER_ROLE) {
File: contracts/cash/token/CashKYCSender.sol 34 function setKYCRequirementGroup( 35 uint256 group 36: ) external override onlyRole(KYC_CONFIGURER_ROLE) { 40 function setKYCRegistry( 41 address registry 42: ) external override onlyRole(KYC_CONFIGURER_ROLE) {
File: contracts/lending/OndoPriceOracle.sol 92 function setFTokenToCToken( 93 address fToken, 94 address cToken 95: ) external override onlyOwner {
File: contracts/lending/OndoPriceOracleV2.sol 130 function setPriceCap( 131 address fToken, 132 uint256 value 133: ) external override onlyOwner { 145 function setFTokenToOracleType( 146 address fToken, 147 OracleType oracleType 148: ) external override onlyOwner { 194 function setFTokenToCToken( 195 address fToken, 196 address cToken 197: ) external override onlyOwner { 233 function setFTokenToChainlinkOracle( 234 address fToken, 235 address newChainlinkOracle 236: ) external override onlyOwner { 309 function setMaxChainlinkOracleTimeDelay( 310 uint256 _maxChainlinkOracleTimeDelay 311: ) external override onlyOwner {
_msgSender() if not supporting EIP-2771Use msg.sender if the code does not implement EIP-2771 trusted forwarder support
There are 3 instances of this issue:
File: contracts/cash/token/CashKYCSenderReceiver.sol 64: _getKYCStatus(_msgSender()),
File: contracts/cash/token/CashKYCSender.sol 64: _getKYCStatus(_msgSender()),
File: contracts/cash/token/Cash.sol 37: hasRole(TRANSFER_ROLE, _msgSender()),
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [Gā01] | Using calldata instead of memory for read-only arguments in external functions saves gas | 4 | 480 |
| [Gā02] | <array>.length should not be looked up in every loop of a for-loop | 4 | 12 |
| [Gā03] | require()/revert() strings longer than 32 bytes cost extra gas | 2 | - |
| [Gā04] | Using bools for storage incurs overhead | 1 | 17100 |
| [Gā05] | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 2 | 10 |
| [Gā06] | Using private rather than public for constants, saves gas | 18 | - |
| [Gā07] | Use custom errors rather than revert()/require() strings to save gas | 8 | - |
| [Gā08] | Functions guaranteed to revert when called by normal users can be marked payable | 7 | 147 |
Total: 46 instances over 8 issues with 17749 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
calldata instead of memory for read-only arguments in external functions saves gasWhen a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. 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
There are 4 instances of this issue:
File: contracts/cash/token/CashKYCSenderReceiver.sol /// @audit name - (valid but excluded finding) /// @audit symbol - (valid but excluded finding) 46 function initialize( 47 string memory name, 48 string memory symbol, 49 address kycRegistry, 50 uint256 kycRequirementGroup 51: ) public initializer {
File: contracts/cash/token/CashKYCSender.sol /// @audit name - (valid but excluded finding) /// @audit symbol - (valid but excluded finding) 46 function initialize( 47 string memory name, 48 string memory symbol, 49 address kycRegistry, 50 uint256 kycRequirementGroup 51: ) public initializer {
<array>.length should not be looked up in every loop of a for-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD (3 gas)CALLDATALOAD (3 gas)Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
There are 4 instances of this issue:
File: contracts/cash/CashManager.sol /// @audit (valid but excluded finding) 961: for (uint256 i = 0; i < exCallData.length; ++i) {
File: contracts/cash/factory/CashFactory.sol /// @audit (valid but excluded finding) 127: for (uint256 i = 0; i < exCallData.length; ++i) {
File: contracts/cash/factory/CashKYCSenderFactory.sol /// @audit (valid but excluded finding) 137: for (uint256 i = 0; i < exCallData.length; ++i) {
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol /// @audit (valid but excluded finding) 137: for (uint256 i = 0; i < exCallData.length; ++i) {
require()/revert() strings longer than 32 bytes cost extra gasEach extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas
There are 2 instances of this issue:
File: contracts/cash/factory/CashFactory.sol /// @audit (valid but excluded finding) 152: require(msg.sender == guardian, "CashFactory: You are not the Guardian");
File: contracts/cash/kyc/KYCRegistry.sol /// @audit (valid but excluded finding) 87: require(v == 27 || v == 28, "KYCRegistry: invalid v value in signature");
bools for storage incurs overhead// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past
There is 1 instance of this issue:
File: contracts/cash/kyc/KYCRegistry.sol /// @audit (valid but excluded finding) 39: mapping(uint256 => mapping(address => bool)) public kycState;
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 5 gas per loop
There are 2 instances of this issue:
File: contracts/cash/kyc/KYCRegistry.sol /// @audit (valid but excluded finding) 163: for (uint256 i = 0; i < length; i++) { /// @audit (valid but excluded finding) 180: for (uint256 i = 0; i < length; i++) {
private rather than public for constants, saves gasIf needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table
There are 18 instances of this issue:
File: contracts/cash/CashManager.sol /// @audit (valid but excluded finding) 89: uint256 public constant BPS_DENOMINATOR = 10_000; /// @audit (valid but excluded finding) 122: bytes32 public constant MANAGER_ADMIN = keccak256("MANAGER_ADMIN"); /// @audit (valid but excluded finding) 123: bytes32 public constant PAUSER_ADMIN = keccak256("PAUSER_ADMIN"); /// @audit (valid but excluded finding) 124: bytes32 public constant SETTER_ADMIN = keccak256("SETTER_ADMIN");
File: contracts/cash/factory/CashFactory.sol /// @audit (valid but excluded finding) 44: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE"); /// @audit (valid but excluded finding) 45: bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE"); /// @audit (valid but excluded finding) 46: bytes32 public constant DEFAULT_ADMIN_ROLE = bytes32(0);
File: contracts/cash/factory/CashKYCSenderFactory.sol /// @audit (valid but excluded finding) 44: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE"); /// @audit (valid but excluded finding) 45: bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE"); /// @audit (valid but excluded finding) 46: bytes32 public constant DEFAULT_ADMIN_ROLE = bytes32(0);
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol /// @audit (valid but excluded finding) 44: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE"); /// @audit (valid but excluded finding) 45: bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE"); /// @audit (valid but excluded finding) 46: bytes32 public constant DEFAULT_ADMIN_ROLE = bytes32(0);
File: contracts/cash/kyc/KYCRegistry.sol /// @audit (valid but excluded finding) 31 bytes32 public constant _APPROVAL_TYPEHASH = 32 keccak256( 33 "KYCApproval(uint256 kycRequirementGroup,address user,uint256 deadline)" 34: ); /// @audit (valid but excluded finding) 36: bytes32 public constant REGISTRY_ADMIN = keccak256("REGISTRY_ADMIN");
File: contracts/cash/token/CashKYCSenderReceiver.sol /// @audit (valid but excluded finding) 26 bytes32 public constant KYC_CONFIGURER_ROLE = 27: keccak256("KYC_CONFIGURER_ROLE");
File: contracts/cash/token/CashKYCSender.sol /// @audit (valid but excluded finding) 26 bytes32 public constant KYC_CONFIGURER_ROLE = 27: keccak256("KYC_CONFIGURER_ROLE");
File: contracts/cash/token/Cash.sol /// @audit (valid but excluded finding) 22: bytes32 public constant TRANSFER_ROLE = keccak256("TRANSFER_ROLE");
revert()/require() strings to save gasCustom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas
There are 8 instances of this issue:
File: contracts/cash/CashManager.sol /// @audit (valid but excluded finding) 965: require(success, "Call Failed");
File: contracts/cash/factory/CashFactory.sol /// @audit (valid but excluded finding) 131: require(success, "Call Failed"); /// @audit (valid but excluded finding) 152: require(msg.sender == guardian, "CashFactory: You are not the Guardian");
File: contracts/cash/factory/CashKYCSenderFactory.sol /// @audit (valid but excluded finding) 141: require(success, "Call Failed");
File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol /// @audit (valid but excluded finding) 141: require(success, "Call Failed");
File: contracts/cash/kyc/KYCRegistry.sol /// @audit (valid but excluded finding) 87: require(v == 27 || v == 28, "KYCRegistry: invalid v value in signature"); /// @audit (valid but excluded finding) 92: require(block.timestamp <= deadline, "KYCRegistry: signature expired");
File: contracts/lending/OndoPriceOracleV2.sol /// @audit (valid but excluded finding) 297: require(answer >= 0, "Price cannot be negative");
payableIf a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
There are 7 instances of this issue:
File: contracts/cash/CashManager.sol /// @audit (valid but excluded finding) 526: function pause() external onlyRole(PAUSER_ADMIN) { /// @audit (valid but excluded finding) 533: function unpause() external onlyRole(MANAGER_ADMIN) { /// @audit (valid but excluded finding) 596: function setMintLimit(uint256 _mintLimit) external onlyRole(MANAGER_ADMIN) {
File: contracts/lending/OndoPriceOracle.sol /// @audit (valid but excluded finding) 80: function setPrice(address fToken, uint256 price) external override onlyOwner { /// @audit (valid but excluded finding) 106: function setOracle(address newOracle) external override onlyOwner {
File: contracts/lending/OndoPriceOracleV2.sol /// @audit (valid but excluded finding) 163: function setPrice(address fToken, uint256 price) external override onlyOwner { /// @audit (valid but excluded finding) 182: function setOracle(address newOracle) external override onlyOwner {
#0 - c4-judge
2023-01-23T11:14:54Z
trust1995 marked the issue as grade-b