Debt DAO contest - RedOneN's results

Lines of code

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

Vulnerability details

Impact

The receiveTokenOrETH() function requires that any ETH value sent by the user is never lower than the "amount" variable. However, the contract does consider the case where the user send a value higher than the requested amount. Since "msg.value" could be different than the requested "amount", user could "loose fund" by sending an "excessive ETH amount". Indeed, most of the functions calling "receiveTokenOrETH()" update state variables based on the "amount" value. eg. "credit.deposit" is incremented with "amount" value and not "msg.value", _createCredit and _repay() are feed with "amount" and not "msg.value"...

Proof of Concept

see links to affected code.

Tools Used

Manual audit

Recommended Mitigation Steps

consider using a more restrictive condition :

if(msg.value != amount) { revert TransferFailed(); }

Lines of code

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

Vulnerability details

Impact

It has been observed that Debt DAO is using the deprecated transfer() function for "ETH" transfers through the "sendOutTokenOrETH()" function.

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:

    1)      The claimer smart contract does not implement a payable function.
    2)      The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.
    3)      The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call’s gas usage above 2300.

For all these reasons, transfer() it is not recommended to use this function anymore

Proof of Concept

See link to affected code.

Tools Used

manual audit

Recommended Mitigation Steps

I recommend using call() instead of transfer().

[L-1] Unused/Empty RECEIVE()/FALLBACK() function.

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth))). Having no access control on the function means that someone may send Ether to the contract, and have no way to get anything back out, which is a loss of funds.

File : SpigotedLine.sol

SpigotedLine.sol#L272

[N-1] Use a more recent version of solidity.

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. Moreover, v0.8.10 has a series of bug. eg. fix constructor source mappings for immutable, fix internal error when function names are not unique, improved error message for constant variables with mapping types ...

[N-2] Use Scientific notation (e.g. 1E18) rather than Exponentiation (e.g. 10**18)

Scientific notation should be used for better code readability

File : EscrowLib.sol

EscrowLib.sol#L42

[N-3] Large multiples of ten should use scientific notation

Use (e.g. 1e6) rather than decimal literals (e.g. 1000000), for better code readability

File : InterestRateCredit.sol

InterestRateCredit.sol#L9 InterestRateCredit.sol#L10

[N-4] Use of block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

File : LineOfCredit.sol

LineOfCredit.sol#L58 LineOfCredit.sol#L132

File : InterestRateCredit.sol

InterestRateCredit.sol#L48 InterestRateCredit.sol#L50 InterestRateCredit.sol#L82

[G-1] ++i/i++ should be UNCHECKED{++i}/UNCHECKED{i++} when it is not possible for them to overflow (eg. 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 around 40 gas per loop.

File : CreditListLib.sol

CreditListLib.sol#L23 CreditListLib.sol#L51

File : EscrowLib.sol

EscrowLib.sol#L57

File : LineOfCredit.sol

LineOfCredit.sol#L179 LineOfCredit.sol#L203 LineOfCredit.sol#L520

[G-2] Usage of UINT/INT smaller than 256bits 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.

File : SpigotedLineLib.sol

SpigotedLineLib.sol#L13

File : SpigotedLine.sol

SpigotedLine.sol#L60

File : LineFactory.sol

LineFactory.sol#L12 LineFactory.sol#L13 LineFactory.sol#L14

File : SecuredLine.sol

SecuredLine.sol#L21

File : SpigotLib.sol

SpigotLib.sol#L25

[G-3] Non-strict inequalities are cheaper than strict ones.

Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas). This also holds true between <= and <.

Various instances across the different contracts.

[G-4] Duplicated require()/revert() checks should be refactored to a modifier to save gas.

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

File : EscrowLib.sol

require(amount>0); EscrowLib.sol#L91 EscrowLib.sol#L161 EscrowLib.sol#L198

File : LineOfCredit.sol

require(interestrate.setrate(id,drate,frate)); LineOfCredit.sol#L241 LineOfCredit.sol#L259

[G-5] Empty blocks should be removed or emit something

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{...}}). Empty receive()/fallback() payable functions that are not used, can be removed to save deployment gas.

File : SpigotedLine.sol

SpigotedLine.sol#L272

[G-6] Use a more recent version of solidity.

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.