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: 48/120
Findings: 2
Award: $110.58
🌟 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
61.3462 USDC - $61.35
File: /contracts/modules/credit/LineOfCredit.sol 55: oracle = IOracle(oracle_); 56: arbiter = arbiter_; 57: borrower = borrower_;
File: /contracts/modules/spigot/Spigot.sol 36: state.owner = _owner; 37: state.operator = _operator; 38: state.treasury = _treasury;
File: /contracts/modules/oracle/Oracle.sol 16: registry = FeedRegistryInterface(_registry);
File: /contracts/modules/escrow/Escrow.sol 49: oracle = _oracle; 50: borrower = _borrower;
There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.
Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name.
File: /contracts/utils/CreditLib.sol //@audit: 18 140: decimals = 18; //@audit: 18 145: decimals = !passed ? 18 : abi.decode(result, (uint8));
File: /contracts/modules/factories/LineFactory.sol 140: // TODO: test 145: // TODO: test
Require statements should have descriptive reasons to aid in understanding why a particular action failed.
File: /contracts/modules/credit/LineOfCredit.sol 112: require(uint(status) >= uint( LineLib.STATUS.ACTIVE)); 241: require(interestRate.setRate(id, drate, frate)); 259: require(interestRate.setRate(id, drate, frate)); 326: require(amount <= credit.principal + credit.interestAccrued);
File: /contracts/modules/credit/SpigotedLine.sol 62: require(defaultRevenueSplit_ <= SpigotedLineLib.MAX_SPLIT); 143: require(amount <= unusedTokens[credit.token]); 160: require(msg.sender == borrower); 239: require(msg.sender == arbiter);
File: /contracts/modules/credit/EscrowedLine.sol 64: require(escrow_.liquidate(amount, targetToken, to)); 90: require(escrow.updateLine(newLine));
File: /contracts/utils/SpigotedLineLib.sol 147: require(ISpigot(spigot).updateOwner(newLine));
File: /contracts/utils/EscrowLib.sol 91: require(amount > 0); 105: require(msg.sender == ILineOfCredit(self.line).arbiter()); 161: require(amount > 0); 198: require(amount > 0);
File: /contracts/utils/SpigotedLineLib.sol //@audit: Missing @param amount, @param sellToken, @param swapTarget,@param zeroExTradeData 120: function trade( 121: uint256 amount, 122: address sellToken, 123: address payable swapTarget, 124: bytes calldata zeroExTradeData 125: ) 126: public 127: returns(bool) //@audit: Missing @param spigot, @param newLine 146: function rollover(address spigot, address newLine) external returns(bool) { //@audit: Missing @param spigot, @param arbiter 151: function canDeclareInsolvent(address spigot, address arbiter) external view returns (bool) { //@audit: Missing @param status, @param defaultSplit 169: function updateSplit(address spigot, address revenueContract, LineLib.STATUS status, uint8 defaultSplit) external returns (bool) { //@audit: Missing @param spigot, @param status, @param borrower, @param arbiter, @param to 194: function releaseSpigot(address spigot, LineLib.STATUS status, address borrower, address arbiter, address to) external returns (bool) { //@audit: Missing @param to, @param token, @param amount, @param status, @param borrower, @param arbiter 217: function sweep(address to, address token, uint256 amount, LineLib.STATUS status, address borrower, address arbiter) external returns (bool) {
File: /contracts/utils/CreditLib.sol //@audit: Missing @param credit, @param id, @param oracle, @param interestRate //@audit: Missing @return c, @return principal,@return interest 73: function getOutstandingDebt( 74: ILineOfCredit.Credit memory credit, 75: bytes32 id, 76: address oracle, 77: address interestRate 78: ) 79: external 80: returns (ILineOfCredit.Credit memory c, uint256 principal, uint256 interest) 81: { //@audit: Missing @param id, @param amount, @param lender, @param token //@audit: Missing @return credit 125: function create( 126: bytes32 id, 127: uint256 amount, 128: address lender, 129: address token, 130: address oracle 131: ) 132: external 133: returns(ILineOfCredit.Credit memory credit)
File:/contracts/modules/interest-rate/InterestRateCredit.sol //@audit: Missing @param id, @param drawnBalance, @param facilityBalance 34: function accrueInterest( 35: bytes32 id, 36: uint256 drawnBalance, 37: uint256 facilityBalance 38: ) external override onlyLineContract returns (uint256) { //@audit: Missing @param id, @param dRate, @param fRate 74: function setRate( 75: bytes32 id, 76: uint128 dRate, 77: uint128 fRate 78: ) external onlyLineContract returns (bool) {
This is a best-practice to protect against reentrancy in other modifiers
File: /contracts/modules/credit/SpigotedLine.sol 193: function claimAndRepay(address claimToken, bytes calldata zeroExTradeData) 194: external 195: whileBorrowing 196: nonReentrant 197: returns (uint256) 198: {
File: /contracts/modules/credit/SpigotedLine.sol 154: function claimAndTrade(address claimToken, bytes calldata zeroExTradeData) 155: external 156: whileBorrowing 157: nonReentrant 158: returns (uint256) 159: {
#0 - c4-judge
2022-12-07T17:54:55Z
dmvt marked the issue as grade-b
🌟 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
49.2315 USDC - $49.23
NB: Some functions have been truncated where neccessary to just show affected parts of the code Throught the report some places might be denoted with audit tags to show the actual place affected.
The code can be optimized by minimizing the number of SLOADs.
SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.
File: /contracts/utils/EscrowLib.sol 152: function releaseCollateral( 160: ) external returns (uint256) { 163: if(self.deposited[token].amount < amount) { revert InvalidCollateral(); } //@audit: Initial access 164: self.deposited[token].amount -= amount;//@audit: second access
diff --git a/contracts/utils/EscrowLib.sol b/contracts/utils/EscrowLib.sol index e838d7f..cfb4a9c 100644 --- a/contracts/utils/EscrowLib.sol +++ b/contracts/utils/EscrowLib.sol @@ -160,8 +160,9 @@ library EscrowLib { ) external returns (uint256) { require(amount > 0); if(msg.sender != borrower) { revert CallerAccessDenied(); } - if(self.deposited[token].amount < amount) { revert InvalidCollateral(); } - self.deposited[token].amount -= amount; + uint _depositedAmount = self.deposited[token].amount; + if(_depositedAmount < amount) { revert InvalidCollateral(); } + self.deposited[token].amount = _depositedAmount - amount; LineLib.sendOutTokenOrETH(token, to, amount);
File: /contracts/utils/EscrowLib.sol 192: function liquidate( 200: if(self.deposited[token].amount < amount) { revert InvalidCollateral(); } 202: self.deposited[token].amount -= amount;
diff --git a/contracts/utils/EscrowLib.sol b/contracts/utils/EscrowLib.sol index e838d7f..92d9aef 100644 --- a/contracts/utils/EscrowLib.sol +++ b/contracts/utils/EscrowLib.sol @@ -197,9 +197,10 @@ library EscrowLib { ) external returns (bool) { require(amount > 0); if(msg.sender != self.line) { revert CallerAccessDenied(); } - if(self.deposited[token].amount < amount) { revert InvalidCollateral(); } + uint _depositedAmount = self.deposited[token].amount; + if(_depositedAmount < amount) { revert InvalidCollateral(); } - self.deposited[token].amount -= amount; + self.deposited[token].amount = _depositedAmount - amount; LineLib.sendOutTokenOrETH(token, to, amount);
File: /contracts/modules/credit/SpigotedLine.sol 137: function useAndRepay(uint256 amount) external whileBorrowing returns(bool) { 143: require(amount <= unusedTokens[credit.token]); 144: unusedTokens[credit.token] -= amount;
diff --git a/contracts/modules/credit/SpigotedLine.sol b/contracts/modules/credit/SpigotedLine.sol index 8c91716..748f190 100644 --- a/contracts/modules/credit/SpigotedLine.sol +++ b/contracts/modules/credit/SpigotedLine.sol @@ -140,8 +140,9 @@ contract SpigotedLine is ISpigotedLine, LineOfCredit, ReentrancyGuard { if (msg.sender != borrower && msg.sender != credit.lender) { revert CallerAccessDenied(); } - require(amount <= unusedTokens[credit.token]); - unusedTokens[credit.token] -= amount; + uint256 _unusedTokens = unusedTokens[credit.token]; + require(amount <= _unusedTokens); + unusedTokens[credit.token] = _unusedTokens - amount; credits[id] = _repay(_accrue(credit, id), id, amount);
Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
Affected code:
File: /contracts/modules/interest-rate/InterestRateCredit.sol 42: function _accrueInterest( 43: bytes32 id, 44: uint256 drawnBalance, 45: uint256 facilityBalance 46: ) internal returns (uint256) {
The above function is only called once on Line 39
File: /contracts/modules/credit/LineOfCredit.sol 167: function _updateOutstandingDebt() 168: internal 169: returns (uint256 principal, uint256 interest) 170: { 435: function _createCredit( 436: address lender, 437: address token, 438: uint256 amount 439: ) 440: internal 516: function _sortIntoQ(bytes32 p) internal returns (bool) {
Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value
File: /contracts/utils/CreditLib.sol 84: int256 price = IOracle(oracle).getLatestAnswer(c.token); 135: int price = IOracle(oracle).getLatestAnswer(token); 251: uint256 accruedToken = IInterestRateCredit(interest).accrueInterest(
File: /contracts/modules/factories/LineFactory.sol 64: address s = factory.deploySpigot(address(this), borrower, borrower); 65: address e = factory.deployEscrow( 66: defaultMinCRatio, 67: oracle, 68: address(this), 69: borrower 70: ); 96: address s = factory.deploySpigot( 97: address(this), 98: coreParams.borrower, 99: coreParams.borrower 100: ); 101: address e = factory.deployEscrow( 102: coreParams.cratio, 103: oracle, 104: address(this), 105: coreParams.borrower 106: );
When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
File: /contracts/modules/credit/LineOfCredit.sol //@audit: int128 drate,uint128 frate 223: function addCredit( 224: uint128 drate, 225: uint128 frate, 226: uint256 amount, 227: address token, 228: address lender 229: ) //@audit: uint128 drate,uint128 frate 247: function setRates( 248: bytes32 id, 249: uint128 drate, 250: uint128 frate 251: )
File: /contracts/utils/LineFactoryLib.sol //@audit: uint8 revenueSplit 77: function deploySecuredLine( 78: address oracle, 79: address arbiter, 80: address borrower, 81: address payable swapTarget, 82: address s, 83: address e, 84: uint ttl, 86: uint8 revenueSplit 87: ) external returns(address){
File: /contracts/utils/SpigotedLineLib.sol //@audit: uint8 defaultSplit 169: function updateSplit(address spigot, address revenueContract, LineLib.STATUS status, uint8 defaultSplit) external returns (bool) {
File: /contracts/utils/CreditLib.sol //@audit: uint8 decimals 109: function calculateValue( 110: int price, 111: uint256 amount, 112: uint8 decimals 113: )
File: /contracts/modules/factories/LineFactory.sol //@audit: uint32 minCRatio 42: function deployEscrow( 43: uint32 minCRatio, 44: address owner, 45: address borrower 46: ) external returns (address) {
File: /contracts/modules/interest-rate/InterestRateCredit.sol //@audit: uint128 dRate,uint128 fRate 74: function setRate( 75: bytes32 id, 76: uint128 dRate, 77: uint128 fRate 78: ) external onlyLineContract returns (bool) {
Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block see resource
File: /contracts/utils/SpigotedLineLib.sol 101: uint256 diff = oldClaimTokens - newClaimTokens;
diff --git a/contracts/utils/SpigotedLineLib.sol b/contracts/utils/SpigotedLineLib.sol index ac86b45..a46d9e8 100644 --- a/contracts/utils/SpigotedLineLib.sol +++ b/contracts/utils/SpigotedLineLib.sol @@ -98,7 +98,10 @@ library SpigotedLineLib { // used reserve revenue to repay debt if(oldClaimTokens > newClaimTokens) { - uint256 diff = oldClaimTokens - newClaimTokens; + uint256 diff; + unchecked { + diff = oldClaimTokens - newClaimTokens; + } // used more tokens than we had in revenue reserves. // prevent borrower from pulling idle lender funds to repay other lenders
The above operation cannot underflow due to the check on Line 100
that checks that the value of oldClaimTokens is greater than newClaimTokens before perfoming the operation
File: /contracts/utils/SpigotedLineLib.sol 109: unused - diff
The above operation cannot underflow due to the check on Line 105
that checks that the value of unused is greater than diff before perfoming the operation
File: /contracts/utils/EscrowLib.sol 164: self.deposited[token].amount -= amount;
The above operation cannot underflow due to the check on Line 163
that checks that the value of self.deposited[token].amount is greater than amount before perfoming the operation
File: /contracts/utils/EscrowLib.sol 202: self.deposited[token].amount -= amount;
The above operation cannot underflow due to the check on Line 200
that checks that the value of self.deposited[token].amount is greater than amount before perfoming the operation
File: /contracts/modules/credit/SpigotedLine.sol 122: unusedTokens[credit.token] -= repaid - newTokens;
The operation repaid - newTokens cannot underflow due to the check on Line 120 that ensures that repaid is greater than newTokens before perfoming the subtraction
File: /contracts/modules/credit/SpigotedLine.sol 125: unusedTokens[credit.token] += newTokens - repaid;
The operation newTokens - repaid would only be performed if newTokens is greater than repaid
File: /contracts/modules/credit/SpigotedLine.sol 144: unusedTokens[credit.token] -= amount;
The operation unusedTokens[credit.token] - amount cannot underflow as we have a check on Line 143 that ensures that the value of unusedTokens[credit.token] is greater than that of amount
The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop . eg.
e.g Let's work with a sample loop below.
for(uint256 i; i < 10; i++){ //doSomething }
can be written as shown below.
for(uint256 i; i < 10;) { // loop logic unchecked { i++; } }
We can also write it as an inlined function like below.
function inc(i) internal pure returns (uint256) { unchecked { return i + 1; } } for(uint256 i; i < 10; i = inc(i)) { // doSomething }
Affected code
File: /contracts/modules/credit/LineOfCredit.sol for (uint256 i; i < len; ++i) { id = ids[i]; // null element in array from closing a position. skip for gas savings if(id == bytes32(0)) { continue; } (Credit memory c, uint256 _p, uint256 _i) = CreditLib.getOutstandingDebt( credits[id], id, oracle_, interestRate_ ); // update total outstanding debt principal += _p; interest += _i; // save changes to storage credits[id] = c; }
diff --git a/contracts/modules/credit/LineOfCredit.sol b/contracts/modules/credit/LineOfCredit.sol index 21ef853..b6c6e36 100644 --- a/contracts/modules/credit/LineOfCredit.sol +++ b/contracts/modules/credit/LineOfCredit.sol @@ -176,7 +176,7 @@ contract LineOfCredit is ILineOfCredit, MutualConsent { address oracle_ = address(oracle); // gas savings address interestRate_ = address(interestRate); // gas savings - for (uint256 i; i < len; ++i) { + for (uint256 i; i < len;) { id = ids[i]; // null element in array from closing a position. skip for gas savings @@ -193,6 +193,9 @@ contract LineOfCredit is ILineOfCredit, MutualConsent { interest += _i; // save changes to storage credits[id] = c; + unchecked { + ++i; + } } }
File:/contracts/modules/credit/LineOfCredit.sol 203: for (uint256 i; i < len; ++i) { 204: id = ids[i]; 205: Credit memory credit = credits[id]; 206: credits[id] = _accrue(credit, id); 207: }
diff --git a/contracts/modules/credit/LineOfCredit.sol b/contracts/modules/credit/LineOfCredit.sol index 21ef853..0360364 100644 --- a/contracts/modules/credit/LineOfCredit.sol +++ b/contracts/modules/credit/LineOfCredit.sol @@ -200,10 +200,13 @@ contract LineOfCredit is ILineOfCredit, MutualConsent { function accrueInterest() external override returns(bool) { uint256 len = ids.length; bytes32 id; - for (uint256 i; i < len; ++i) { + for (uint256 i; i < len;) { id = ids[i]; Credit memory credit = credits[id]; credits[id] = _accrue(credit, id); + unchecked { + ++i; + } } return true;
File: /contracts/modules/credit/LineOfCredit.sol for (uint256 i; i <= lastSpot; ++i) { }
diff --git a/contracts/modules/credit/LineOfCredit.sol b/contracts/modules/credit/LineOfCredit.sol index 21ef853..0aca439 100644 --- a/contracts/modules/credit/LineOfCredit.sol +++ b/contracts/modules/credit/LineOfCredit.sol @@ -517,7 +517,7 @@ contract LineOfCredit is ILineOfCredit, MutualConsent { uint256 lastSpot = ids.length - 1; uint256 nextQSpot = lastSpot; bytes32 id; - for (uint256 i; i <= lastSpot; ++i) { + for (uint256 i; i <= lastSpot;) { id = ids[i]; if (p != id) { if ( @@ -533,6 +533,10 @@ contract LineOfCredit is ILineOfCredit, MutualConsent { ids[nextQSpot] = p; // p put at target index return true; } + + unchecked { + ++i; + } } }
File: /contracts/utils/EscrowLib.sol 57: for (uint256 i; i < length; ++i) { 81: }
diff --git a/contracts/utils/EscrowLib.sol b/contracts/utils/EscrowLib.sol index e838d7f..8c945c4 100644 --- a/contracts/utils/EscrowLib.sol +++ b/contracts/utils/EscrowLib.sol @@ -54,7 +54,7 @@ library EscrowLib { uint256 length = self.collateralTokens.length; IOracle o = IOracle(oracle); IEscrow.Deposit memory d; - for (uint256 i; i < length; ++i) { + for (uint256 i; i < length;) { address token = self.collateralTokens[i]; d = self.deposited[token]; // new var so we don't override original deposit amount for 4626 tokens @@ -78,6 +78,9 @@ library EscrowLib { d.assetDecimals ); } + unchecked { + ++i; + } } return collateralValue;
File: /contracts/utils/CreditListLib.sol 23: for(uint256 i; i < len; ++i) { 29: }
diff --git a/contracts/utils/CreditListLib.sol b/contracts/utils/CreditListLib.sol index 1c378e4..28b062a 100644 --- a/contracts/utils/CreditListLib.sol +++ b/contracts/utils/CreditListLib.sol @@ -20,12 +20,14 @@ library CreditListLib { function removePosition(bytes32[] storage ids, bytes32 id) external returns(bool) { uint256 len = ids.length; - for(uint256 i; i < len; ++i) { + for(uint256 i; i < len;) { if(ids[i] == id) { delete ids[i]; return true; } - + unchecked { + ++i; + } } return true;
File: /contracts/utils/CreditListLib.sol 51: for(uint i = 1; i < len; ++i) { 52: ids[i - 1] = ids[i]; // could also clean arr here like in _SortIntoQ 54: }
Since the operation len - 1 cannot underflow we can wrap the whole loop with unchecked blocks
diff --git a/contracts/utils/CreditListLib.sol b/contracts/utils/CreditListLib.sol index 1c378e4..749dcfc 100644 --- a/contracts/utils/CreditListLib.sol +++ b/contracts/utils/CreditListLib.sol @@ -48,12 +48,14 @@ library CreditListLib { ids[1] = last; } else { // move all existing ids up in the queue - for(uint i = 1; i < len; ++i) { - ids[i - 1] = ids[i]; // could also clean arr here like in _SortIntoQ + unchecked { + for(uint i = 1; i < len; ++i) { + ids[i - 1] = ids[i]; // could also clean arr here like in _SortIntoQ - } + } // cycle first id back to end of queue - ids[len - 1] = last; + ids[len - 1] = last; + } } return true;
Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.Each extra chunk of byetes past the original 32 incurs an MSTORE which costs 3 gas
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
File: /contracts/modules/interest-rate/InterestRateCredit.sol 26: require( 27: msg.sender == lineContract, 28: "InterestRateCred: only line contract." 29: );
#0 - c4-judge
2022-11-17T23:07:06Z
dmvt marked the issue as grade-b