Debt DAO contest - erictee's results

[L-01] approve() and safeApprove() should be replaced with safeIncreaseAllowance() / safeDecreaseAllowance()

Impact

approve() & safeApprove() are deprecated and subject to a known front-running attack. Consider using safeIncreaseAllowance() & safeDecreaseAllowance() instead.

Findings:

contracts/utils/SpigotedLineLib.sol:L134                     IERC20(sellToken).approve(swapTarget, amount);

[L-02] Avoid floating pragmas for non-library contracts.

Impact

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.

Findings:

contracts/mock/ZeroEx.sol:L1     pragma solidity ^0.8.9;

contracts/modules/credit/SpigotedLine.sol:L1     pragma solidity ^0.8.9;

contracts/modules/credit/SecuredLine.sol:L1     pragma solidity ^0.8.9;

contracts/modules/credit/LineOfCredit.sol:L1     pragma solidity ^0.8.9;

contracts/modules/oracle/Oracle.sol:L2     pragma solidity ^0.8.9;

contracts/modules/interest-rate/InterestRateCredit.sol:L1     pragma solidity ^0.8.9;

[L-03] require()/revert() statements should have descriptive strings.

Impact

Consider adding descriptive strings in require()/revert().

Findings:

contracts/utils/SpigotedLineLib.sol:L147      require(ISpigot(spigot).updateOwner(newLine));

contracts/utils/SpigotLib.sol:L96            require(LineLib.sendOutTokenOrETH(token, self.treasury, claimed - escrowedAmount));

contracts/utils/SpigotLib.sol:L128        require(revenueContract != address(this));

contracts/utils/SpigotLib.sol:L130        require(self.settings[revenueContract].transferOwnerFunction == bytes4(0));

contracts/utils/SpigotLib.sol:L155        require(success);

contracts/utils/SpigotLib.sol:L180        require(newOwner != address(0));

contracts/utils/SpigotLib.sol:L189        require(newOperator != address(0));

contracts/utils/SpigotLib.sol:L201        require(newTreasury != address(0));

contracts/utils/EscrowLib.sol:L91        require(amount > 0);

contracts/utils/EscrowLib.sol:L105        require(msg.sender == ILineOfCredit(self.line).arbiter());

contracts/utils/EscrowLib.sol:L161        require(amount > 0);

contracts/utils/EscrowLib.sol:L198        require(amount > 0);

contracts/utils/EscrowLib.sol:L216      require(msg.sender == self.line);

contracts/mock/MockLine.sol:L39        require(msg.sender == arbiter);

contracts/modules/credit/SpigotedLine.sol:L62        require(defaultRevenueSplit_ <= SpigotedLineLib.MAX_SPLIT);

contracts/modules/credit/SpigotedLine.sol:L143      require(amount <= unusedTokens[credit.token]);

contracts/modules/credit/SpigotedLine.sol:L160        require(msg.sender == borrower);

contracts/modules/credit/SpigotedLine.sol:L239        require(msg.sender == arbiter);

contracts/modules/credit/LineOfCredit.sol:L112        require(uint(status) >= uint( LineLib.STATUS.ACTIVE));

contracts/modules/credit/LineOfCredit.sol:L241        require(interestRate.setRate(id, drate, frate));

contracts/modules/credit/LineOfCredit.sol:L259        require(interestRate.setRate(id, drate, frate));

contracts/modules/credit/LineOfCredit.sol:L326        require(amount <= credit.principal + credit.interestAccrued);

contracts/modules/credit/EscrowedLine.sol:L64    require(escrow_.liquidate(amount, targetToken, to));

contracts/modules/credit/EscrowedLine.sol:L90    require(escrow.updateLine(newLine));

[L-04] _safemint() should be used rather than _mint() wherever possible

Impact

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function

Findings:

contracts/mock/RevenueToken.sol:L8        _mint(account, amount);

[L-05] zero-address checks are missing

Impact

Zero-address checks are a best practice for input validation of critical address parameters. Accidental use of zero-addresses may result in exceptions, burn fees/tokens, or force redeployment of contracts.

Findings:

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/mock/MockEscrowedLine.sol#L20-L21 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/mock/MockLine.sol#L16 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/mock/MockLine.sol#L21 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/mock/MockLine.sol#L26 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/mock/RevenueToken4626.sol#L11 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/mock/RevenueToken4626.sol#L16 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/mock/SimpleOracle.sol#L12-L13 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/mock/SimpleRevenueContract.sol#L10-L11 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/mock/SimpleRevenueContract.sol#L40 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/EscrowedLine.sol#L17 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L55-L57 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/SecuredLine.sol#L23-L27 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/SecuredLine.sol#L30 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/SpigotedLine.sol#L61 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/SpigotedLine.sol#L64 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/SpigotedLine.sol#L66 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/escrow/Escrow.sol#L49-L51 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/oracle/Oracle.sol#L16 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/spigot/Spigot.sol#L36-L38

[N-01] Open TODOs

Impact

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment.

Findings:

contracts/modules/factories/LineFactory.sol:L140        // TODO: test

contracts/modules/factories/LineFactory.sol:L145        // TODO: test

[G-01] abi.encode() is less efficient than abi.encodePacked()

Impact

Consider using abi.encodePacked() instead for efficieny.

Findings:

contracts/utils/CreditLib.sol:L69    return keccak256(abi.encode(line, lender, token));

[G-02] Use custom errors rather than revert()/require() string to save gas

Impact

Custom errors are available from solidity version 0.8.4, it saves around 50 gas each time they are hit by avoiding having to allocate and store the revert string.

Findings:

contracts/mock/SimpleOracle.sol:L26        require(prices[token] != 0, "SimpleOracle: unsupported token");

contracts/mock/SimpleRevenueContract.sol:L15        require(msg.sender == owner, "Revenue: Only owner can claim");

contracts/mock/SimpleRevenueContract.sol:L17            require(revenueToken.transfer(owner, revenueToken.balanceOf(address(this))), "Revenue: bad transfer");

contracts/mock/SimpleRevenueContract.sol:L26            require(revenueToken.transfer(owner, revenueToken.balanceOf(address(this))), "Revenue: bad transfer");

contracts/mock/SimpleRevenueContract.sol:L34        require(msg.sender == owner, "Revenue: Only owner can operate");

contracts/mock/SimpleRevenueContract.sol:L39        require(msg.sender == owner, "Revenue: Only owner can transfer");

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

Impact

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.

Findings:

contracts/modules/credit/SpigotedLine.sol:L272    receive() external payable {}

[G-04] ++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.

Impact

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.

Findings:

contracts/utils/CreditListLib.sol:L23      for(uint256 i; i < len; ++i) {

contracts/utils/CreditListLib.sol:L51        for(uint i = 1; i < len; ++i) {

contracts/utils/EscrowLib.sol:L57        for (uint256 i; i < length; ++i) {

contracts/modules/credit/LineOfCredit.sol:L179        for (uint256 i; i < len; ++i) {

contracts/modules/credit/LineOfCredit.sol:L203        for (uint256 i; i < len; ++i) {

contracts/modules/credit/LineOfCredit.sol:L520        for (uint256 i; i <= lastSpot; ++i) {

[G-05] Revert message greater than 32 bytes

Impact

Keep revert message lower than or equal to 32 bytes to save gas.

Findings:

contracts/modules/interest-rate/InterestRateCredit.sol:L26        require(