Debt DAO contest - cryptonue's results

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/LineLib.sol#L48

Vulnerability details

Impact

The use of payable.transfer() is heavily frowned upon because it can lead to the locking of funds. The transfer() call requires that the recipient has a payable callback, only provides 2300 gas for its operation. This means the following cases can cause the transfer to fail:

• The contract does not have a payable callback
• The contract’s payable callback spends more than 2300 gas (which is only enough to emit something)
• The contract is called through a proxy which itself uses up the 2300 gas

If a user falls into one of the above categories, they’ll be unable to receive funds. Inaccessible funds means loss of funds, which is Medium severity.

Proof of Concept

The LineLib.sol sendOutTokenOrETH is being use extensively from other contracts, as this is kind of a library for sending token or ETH from the contract to other receiver contract.

File: LineLib.sol
34:     function sendOutTokenOrETH(
35:       address token,
36:       address receiver,
37:       uint256 amount
38:     )
39:       external
40:       returns (bool)
41:     {
42:         if(token == address(0)) { revert TransferFailed(); }
43:         
44:         // both branches revert if call failed
45:         if(token!= Denominations.ETH) { // ERC20
46:             IERC20(token).safeTransfer(receiver, amount);
47:         } else { // ETH
48:             payable(receiver).transfer(amount);
49:         }
50:         return true;
51:     }

Tools Used

Manual analysis

Recommended Mitigation Steps

Use address.call{value:x}() instead.

[L] NO TRANSFER OWNERSHIP PATTERN

Recommend considering implementing a two step process where the owner or admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

File: SpigotLib.sol
178:     function updateOwner(SpigotState storage self, address newOwner) external returns (bool) {
179:         if(msg.sender != self.owner) { revert CallerAccessDenied(); }
180:         require(newOwner != address(0));
181:         self.owner = newOwner;
182:         emit UpdateOwner(newOwner);
183:         return true;
184:     }

[L] AMOUNT NOT CHECKED

When sendOutTokenOrETH() being called, there is no check on amount > 0, it will be better to just revert or skip the transfer if the amount is 0. This will also save gas because no transaction will be executed if amount is 0.

File: LineLib.sol
34:     function sendOutTokenOrETH(
35:       address token,
36:       address receiver,
37:       uint256 amount
38:     )
39:       external
40:       returns (bool)
41:     {
42:         if(token == address(0)) { revert TransferFailed(); }
43:         
44:         // both branches revert if call failed
45:         if(token!= Denominations.ETH) { // ERC20
46:             IERC20(token).safeTransfer(receiver, amount);
47:         } else { // ETH
48:             payable(receiver).transfer(amount);
49:         }
50:         return true;
51:     }

[L] UNSPECIFIC COMPILER VERSION PRAGMA

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.

File: LineOfCredit.sol
1: pragma solidity ^0.8.9;

File: SecuredLine.sol
1: pragma solidity ^0.8.9;

File: SpigotedLine.sol
1: pragma solidity ^0.8.9;

File: InterestRateCredit.sol
1: pragma solidity ^0.8.9;

File: Oracle.sol
2: pragma solidity ^0.8.9;

[L] UNUSED RECEIVE() FUNCTION

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

File: SpigotedLine.sol
272:     receive() external payable {}

File: Spigot.sol
227:     receive() external payable {
228:         return;
229:     }

[L] DECIMALS() NOT PART OF ERC20 STANDARD

decimals() is not part of the official ERC20 standard and might fail for tokens that do not implement it. While in practice it is very unlikely, as usually most of the tokens implement it, this should still be considered as a potential issue.

File: CreditLib.sol
142:           (bool passed, bytes memory result) = token.call(
143:               abi.encodeWithSignature("decimals()")
144:           );

File: EscrowLib.sol
131:                 (bool successDecimals, bytes memory decimalBytes) = deposit
132:                     .asset
133:                     .call(abi.encodeWithSignature("decimals()"));

[NC] USE A MORE RECENT VERSION OF SOLIDITY

Most of the contract use 0.8.9 version of solidity, use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(,) Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

[NC] TYPOS

Any typos in contract code (even for comments) might considered a low quality code/documentation.

• availble
• doesnt
• prinicpal
• itselgf
• pat down debt
• dont
• fuly

[NC] UNUSED CODE COMMENT

It's better to remove commented code if it's not being used again

File: EscrowedLine.sol
10: // import { SecuredLine } from "./SecuredLine.sol";

[NC] LONG LINES

Source codes lines should be limited to a certain number of characters, 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. A good practice is to ensure the code does not require a horizontal scroll bar on GitHub. Even if it's for a comments it's better to follow this limit. 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, longer than that should be split.

[NC] INCONSISTENCY USING UINT AND UINT256

Most of the time it use uint256 but some other part uint. Even though it's the same meaning, but a good consistency is better for a quality code.

++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

The 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.

File: CreditListLib.sol
23:       for(uint256 i; i < len; ++i) {

51:       for(uint i = 1; i < len; ++i) {

USAGE OF UINTS/INTS SMALLER THAN 32 BYTES (256 BITS) INCURS OVERHEAD

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

Each operation involving a uint8 costs an extra 28 gas 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.

File: ILineFactory.sol
04:     struct CoreLineParams {
05:         address borrower;
06:         uint256 ttl;
07:         uint32 cratio;
08:         uint8 revenueSplit;
09:     }

File: ISpigot.sol
5:     struct Setting {
6:         uint8 ownerSplit;             // x/100 % to Owner, rest to Treasury
7:         bytes4 claimFunction;         // function signature on contract to call and claim revenue
8:         bytes4 transferOwnerFunction; // function signature on contract to call and transfer ownership 
9:     }

File: SpigotedLine.sol
32:     uint8 public immutable defaultRevenueSplit;

File: LineFactory.sol
12:     uint8 constant defaultRevenueSplit = 90; // 90% to debt repayment
13:     uint8 constant MAX_SPLIT = 100; // max % to take
14:     uint32 constant defaultMinCRatio = 3000; // 30.00% minimum collateral ratio

INTERNAL FUNCTIONS ONLY CALLED ONCE CAN BE INLINED TO SAVE GAS

When we define internal functions to perform computation:

• The contract’s code size gets bigger
• The function call consumes more gas than executing it as an inlined function (part of the code, without the function call)

When it does not affect readability, it is recommended to inline functions in order to save gas Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

File: LineOfCredit.sol
167:     function _updateOutstandingDebt()
168:         internal
169:         returns (uint256 principal, uint256 interest)
170:     {

File: InterestRateCredit.sol
42:     function _accrueInterest(
43:         bytes32 id,
44:         uint256 drawnBalance,
45:         uint256 facilityBalance
46:     ) internal returns (uint256) {

File: MutualConsent.sol
38:     function _mutualConsent(address _signerOne, address _signerTwo) internal returns(bool) {

MATHEMATICAL OPTIMIZATIONS

X += Y costs 22 more gas than X = X + Y. This can mean a lot of gas wasted in a function call when the computation is repeated n times (loops)

File: LineOfCredit.sol
179:         for (uint256 i; i < len; ++i) {
...
192:             principal += _p;
193:             interest += _i;
...
196:         }

File: EscrowLib.sol
57:         for (uint256 i; i < length; ++i) {
...
62:             if (deposit != 0) {
... 
75:                 collateralValue += CreditLib.calculateValue(
76:                   o.getLatestAnswer(d.asset),
77:                   deposit,
78:                   d.assetDecimals
79:                 );
80:             }
81:         }

MODIFIER INSTEAD OF DUPLICATE REQUIRE

When a require statement is used multiple times, it is cheaper in deployment costs to use a modifier instead.

File: EscrowLib.sol
91:         require(amount > 0);
...
161:         require(amount > 0);
...
198:         require(amount > 0);