Platform: Code4rena
Start Date: 03/11/2022
Pot Size: $115,500 USDC
Total HM: 17
Participants: 120
Period: 7 days
Judge: LSDan
Total Solo HM: 1
Id: 174
League: ETH
Rank: 18/120
Findings: 2
Award: $1,553.44
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0xNazgul, 0xRoxas, 0xSmartContract, Awesome, Aymen0909, B2, BClabs, Bnke0x0, Deekshith99, Deivitto, Diana, Dinesh11G, Funen, HE1M, HardlyCodeMan, Josiah, Nyx, Rahoz, RaymondFam, RedOneN, ReyAdmirado, Rolezn, Saintcode_, TomJ, Trust, __141345__, a12jmx, adriro, ajtra, aphak5010, apostle0x01, brgltd, btk, bulej93, c3phas, carlitox477, catwhiskeys, ch0bu, chaduke, chrisdior4, cryptonue, cryptostellar5, csanuragjain, ctf_sec, delfin454000, djxploit, durianSausage, erictee, fatherOfBlocks, gogo, i_got_hacked, immeas, joestakey, jumpdest7d, lukris02, martin, mcwildy, merlin, minhquanym, oyc_109, pashov, peanuts, pedr02b2, rbserver, rotcivegaf, rvierdiiev, sakman, saneryee, seyni, shark, slowmoses, tnevler, trustindistrust, w0Lfrum, yurahod, zaskoh
620.1198 USDC - $620.12
| Issue | Contexts | |
|---|---|---|
| LOW‑1 | Missing Checks for Address(0x0) | 1 |
| LOW‑2 | Unused receive() Function Will Lock Ether In Contract | 1 |
| LOW‑3 | The Contract Should approve(0) First | 1 |
| LOW‑4 | Missing Contract-existence Checks Before Low-level Calls | 11 |
| LOW‑5 | Missing parameter validation in constructor | 8 |
| LOW‑6 | The nonReentrant modifier should occur before all other modifiers | 3 |
| LOW‑7 | Contracts are not using their OZ Upgradeable counterparts | 30 |
Total: 55 contexts over 7 issues
| Issue | Contexts | |
|---|---|---|
| NC‑1 | Use a more recent version of Solidity | All in-scope contracts |
| NC‑2 | Redundant Cast | 2 |
| NC‑3 | require() / revert() Statements Should Have Descriptive Reason Strings | 18 |
| NC‑4 | Implementation contract may not be initialized | 7 |
| NC‑5 | Avoid Floating Pragmas: The Version Should Be Locked | 5 |
| NC‑6 | Non-usage of specific imports | 1 |
| NC‑7 | Lines are too long | 14 |
Total: 64 contexts over 7 issues
Lack of zero-address validation on address parameters may lead to transaction reverts, waste gas, require resubmission of transactions and may even force contract redeployments in certain cases within the protocol.
function updateLine: address _line 217: self.line = _line;
Consider adding explicit zero-address validation on input parameters of address type.
receive() Function Will Lock Ether In ContractIf the intention is for the Ether to be used, the function should call another function, otherwise it should revert
272: receive() external payable {}
The function should call another function, otherwise it should revert
approve(0) FirstSome tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.
134: IERC20(sellToken).approve(swapTarget, amount);
Approve with a zero amount first before setting the actual amount.
Low-level calls return success if there is no code present at the specified address.
142: (bool passed, bytes memory result) = token.call(
65: (bool success, bytes memory assetAmount) = token.call(
115: (bool passed, bytes memory tokenAddrBytes) = token.call(
133: .call(abi.encodeWithSignature("decimals()"));
58: (bool success, bytes memory returnVal) = spigot.call(
62: (bool success2, bytes memory returnVal2) = escrow.call(
131: (bool success, ) = swapTarget.call{value: amount}(zeroExTradeData);
135: (bool success, ) = swapTarget.call(zeroExTradeData);
43: (bool claimSuccess,) = revenueContract.call(data);
76: (bool success,) = revenueContract.call(data);
149: (bool success,) = revenueContract.call(
In addition to the zero-address checks, add a check to verify that <address>.code.length > 0
constructorSome parameters of constructors are not checked for invalid values.
17: escrow = IEscrow(_escrow);
55: oracle = IOracle(oracle_); 56: arbiter = arbiter_; 57: borrower = borrower_;
14: address oracle_ 15: address arbiter_ 16: address borrower_ 18: address spigot_ 19: address escrow_
54: address oracle_ 55: address arbiter_ 56: address borrower_ 57: address spigot_
48: minimumCollateralRatio = _minimumCollateralRatio; 49: oracle = _oracle; 50: borrower = _borrower; 51: state.line = _line;
26: factory = IModuleFactory(moduleFactory);
16: registry = FeedRegistryInterface(_registry);
36: state.owner = _owner; 37: state.operator = _operator; 38: state.treasury = _treasury;
Validate the parameters.
nonReentrant modifier should occur before all other modifiersCurrently the nonReentrant modifier is not the first to occur, it should occur before all other modifiers.
93: function claimAndRepay(address claimToken, bytes calldata zeroExTradeData) external whileBorrowing nonReentrant returns (uint256) {
154: function claimAndTrade(address claimToken, bytes calldata zeroExTradeData) external whileBorrowing nonReentrant returns (uint256) {
85: function claimEscrow(address token) external nonReentrant returns (uint256 claimed) {
Re-sort modifiers so that the nonReentrant modifier occurs first.
The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.
According to remappings.txt, the standard openzeppelin contracts are used:
openzeppelin/=lib/openzeppelin-contracts/contracts/
4: import { IERC20 } from "openzeppelin/token/ERC20/IERC20.sol"; 5: import {SafeERC20} from "openzeppelin/token/ERC20/utils/SafeERC20.sol";
2: import { IERC20 } from "openzeppelin/token/ERC20/IERC20.sol"; 4: import { ReentrancyGuard } from "openzeppelin/security/ReentrancyGuard.sol";
12: import { IERC20 } from "openzeppelin/token/ERC20/IERC20.sol"; 13: import {SafeERC20} from "openzeppelin/token/ERC20/utils/SafeERC20.sol";
4: import { IERC20 } from "openzeppelin/token/ERC20/IERC20.sol"; 5: import {SafeERC20} from "openzeppelin/token/ERC20/utils/SafeERC20.sol";
3: import { ReentrancyGuard } from "openzeppelin/security/ReentrancyGuard.sol";
4: import { IERC20 } from "openzeppelin/token/ERC20/IERC20.sol"; 5: import {SafeERC20} from "openzeppelin/token/ERC20/utils/SafeERC20.sol";
5: import { IERC20 } from "openzeppelin/token/ERC20/IERC20.sol"; 6: import {SafeERC20} from "openzeppelin/token/ERC20/utils/SafeERC20.sol";
6: import { IERC20 } from "openzeppelin/token/ERC20/IERC20.sol";
3: import { ReentrancyGuard } from "openzeppelin/security/ReentrancyGuard.sol";
Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
pragma solidity 0.8.9; was found to be in use in all in-scope contracts.
Consider updating to a more recent solidity version.
The type of the variable is the same as the type to which the variable is being cast
No need to cast price since if its negative then it will be return 0
117: return price <= 0 ? 0 : (amount * uint(price)) / (1 * 10 ** decimals);
60: address(line)
require() / revert() Statements Should Have Descriptive Reason Strings90: require(escrow.updateLine(newLine));
112: require(uint(status) >= uint( LineLib.STATUS.ACTIVE));
326: require(amount <= credit.principal + credit.interestAccrued);
143: require(amount <= unusedTokens[credit.token]);
160: require(msg.sender == borrower);
239: require(msg.sender == arbiter);
91: require(amount > 0);
105: require(msg.sender == ILineOfCredit(self.line).arbiter());
161: require(amount > 0);
198: require(amount > 0);
216: require(msg.sender == self.line);
147: require(ISpigot(spigot).updateOwner(newLine));
128: require(revenueContract != address(this));
130: require(self.settings[revenueContract].transferOwnerFunction == bytes4(0));
155: require(success);
180: require(newOwner != address(0));
189: require(newOperator != address(0));
201: require(newTreasury != address(0));
OpenZeppelin recommends that the initializer modifier be applied to constructors. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits. https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5
constructor( address oracle_, address arbiter_, address borrower_, uint256 ttl_ ) {
constructor( address oracle_, address arbiter_, address borrower_, address payable swapTarget_, address spigot_, address escrow_, uint ttl_, uint8 defaultSplit_ ) SpigotedLine( oracle_, arbiter_, borrower_, spigot_, swapTarget_, ttl_, defaultSplit_ ) EscrowedLine(escrow_) {
constructor( address oracle_, address arbiter_, address borrower_, address spigot_, address payable swapTarget_, uint256 ttl_, uint8 defaultRevenueSplit_ ) LineOfCredit(oracle_, arbiter_, borrower_, ttl_) {
constructor( uint32 _minimumCollateralRatio, address _oracle, address _line, address _borrower ) {
constructor( address moduleFactory, address arbiter_, address oracle_, address swapTarget_ ) {
constructor() {
constructor(address _registry) {
Avoid floating pragmas for non-library contracts.
While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.
A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.
It is recommended to pin to a concrete compiler version.
Found usage of floating pragmas ^0.8.9 of Solidity in the following contracts:
The current form of relative path import is not recommended for use because it can unpredictably pollute the namespace. Instead, the Solidity docs recommend specifying imported symbols explicitly. https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files
6: import "../../interfaces/IOracle.sol";
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 Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length
35: // id -> Reference ID for a credit line provided by a single Lender for a given token on a Line of Credit
58: // we dont check borrower is same on both lines because borrower might want new address managing new line
28: /// @notice exchange aggregator (mainly 0x router) to trade revenue tokens from a Spigot for credit tokens owed to lenders
23: // the minimum value of the collateral in relation to the outstanding debt e.g. 10% of outstanding debt
8: // Must divide by 100 too offset bps in numerator and divide by another 100 to offset % and get actual token amount
144: // Changes the revenue split between the Treasury and the Owner based upon the status of the Line of Credit
23: /// @notice Emits data re Lender removes funds (principal) - there is no corresponding function, just withdraw()
26: /// @notice Emits data re Lender withdraws interest - there is no corresponding function, just withdraw()
31: /// @notice After accrueInterest runs, emits the amount of interest added to a Borrower's outstanding balance of interest due
1: // forked from https://github.com/IndexCoop/index-coop-smart-contracts/blob/master/contracts/lib/MutualConsent.sol
175: // if Line of Credit is healthy then set the split to the prior agreed default split of revenue tokens
15: // Configurations for revenue contracts related to the split of revenue, access control to claiming revenue tokens and transfer of Spigot ownership
24: // Maximum numerator for Setting.ownerSplit param to ensure that the Owner can't claim more than 100% of revenue
69: // cant claim revenue via operate() because that fucks up accounting logic. Owner shouldn't whitelist it anyway but just in case
#0 - c4-judge
2022-12-06T17:15:30Z
dmvt marked the issue as grade-a
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0xRajkumar, Awesome, Aymen0909, B2, Bnke0x0, Deivitto, Diana, JC, Metatron, Rahoz, RaymondFam, RedOneN, ReyAdmirado, Rolezn, Saintcode_, TomJ, __141345__, ajtra, aphak5010, brgltd, c3phas, ch0bu, chrisdior4, cryptonue, durianSausage, emrekocak, erictee, exolorkistis, gogo, karanctf, lukris02, martin, me_na0mi, oyc_109, peanuts, rotcivegaf, saneryee, seyni, tnevler, zaskoh
933.3247 USDC - $933.32
| Issue | Contexts | |
|---|---|---|
| GAS‑1 | Empty Blocks Should Be Removed Or Emit Something | 1 |
| GAS‑2 | ++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 | 6 |
| GAS‑3 | abi.encode() is less efficient than abi.encodepacked() | 1 |
| GAS‑4 | Use assembly to check for address(0) | 1 |
| GAS‑5 | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 12 |
| GAS‑6 | Optimize names to save gas | 10 |
| GAS‑7 | Use uint256(1)/uint256(2) instead for true and false boolean states | 5 |
| GAS‑8 | Do not calculate constants | 5 |
| GAS‑9 | Structs can be packed into fewer storage slots | 2 |
Total: 80 contexts over 15 issues
The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}})
272: receive() external payable {}
++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
179: for (uint256 i; i < len; ++i) {
203: for (uint256 i; i < len; ++i) {
520: for (uint256 i; i <= lastSpot; ++i) {
23: for(uint256 i; i < len; ++i) {
51: for(uint i = 1; i < len; ++i) {
57: for (uint256 i; i < length; ++i) {
abi.encode() is less efficient than abi.encodepacked()See for more information: https://github.com/ConnorBlockchain/Solidity-Encode-Gas-Comparison
69: return keccak256(abi.encode(line, lender, token));
address(0)Save 6 gas per instance if using assembly to check for address(0)
e.g.
assembly { if iszero(_addr) { mstore(0x00, "AddressZero") revert(0x00, 0x20) } }
81: if(token == address(0)) return 0;
uints/ints smaller than 32 bytes (256 bits) incurs overheadWhen using elements that are smaller than 32 bytes, your contract's gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html
Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed
32: uint8 public immutable defaultRevenueSplit;
24: uint32 public immutable minimumCollateralRatio;
12: uint8 constant defaultRevenueSplit = 90; 13: uint8 constant MAX_SPLIT = 100; 14: uint32 constant defaultMinCRatio = 3000;
71: uint8 split = defaultRevenueSplit;
138: uint8 decimals;
51: uint8 split = SecuredLine(oldLine).defaultRevenueSplit();
13: uint8 constant MAX_SPLIT = 100;
25: uint8 constant MAX_SPLIT = 100;
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. 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
All in-scope contracts
uint256(1)/uint256(2) instead for true and false boolean statesIf you don't use boolean for storage you will avoid Gwarmaccess 100 gas. In addition, state changes of boolean from true to false can cost up to ~20000 gas rather than uint256(2) to uint256(1) that would cost significantly less.
16: mapping(address => bool) enabled;
15: mapping(bytes32 => bool) public mutualConsents;
14: mapping(bytes4 => bool) whitelistedFunctions; // function -> allowed
Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.
11: uint256 constant INTEREST_DENOMINATOR = ONE_YEAR * BASE_DENOMINATOR;
27: uint256 constant MAX_REVENUE = type(uint256).max / MAX_SPLIT;
Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings
struct CoreLineParams { address borrower; uint256 ttl; uint32 cratio; uint8 revenueSplit; }
Can save 2 slots by changing to:
struct CoreLineParams { address borrower; //@audit 160 bits uint32 cratio; //@audit 32 bits uint8 revenueSplit; //@audit 8 bits uint256 ttl; }
struct SpigotState { address owner; address operator; address treasury; // Total amount of revenue tokens escrowed by the Spigot and available to be claimed mapping(address => uint256) escrowed; // token -> amount escrowed // Functions that the operator is allowed to run on all revenue contracts controlled by the Spigot mapping(bytes4 => bool) whitelistedFunctions; // function -> allowed // Configurations for revenue contracts related to the split of revenue, access control to claiming revenue tokens and transfer of Spigot ownership mapping(address => ISpigot.Setting) settings; // revenue contract -> settings }
Can potentially save 1 slot due to mapping of whitelistedFunctions sized bool (8 bits) by putting after address treasury sized 160 bits:
struct SpigotState { address owner; address operator; address treasury; // Functions that the operator is allowed to run on all revenue contracts controlled by the Spigot mapping(bytes4 => bool) whitelistedFunctions; // function -> allowed // Total amount of revenue tokens escrowed by the Spigot and available to be claimed mapping(address => uint256) escrowed; // token -> amount escrowed // Configurations for revenue contracts related to the split of revenue, access control to claiming revenue tokens and transfer of Spigot ownership mapping(address => ISpigot.Setting) settings; // revenue contract -> settings }
#0 - c4-judge
2022-11-17T22:52:05Z
dmvt marked the issue as grade-a