Reentrancy in _close()

[code423n4]

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L483-L507

Vulnerability details

Impact

The contract fund could be drained due to reentrancy possibility in _close().

Proof of Concept

In _close() the token transfer happens before the storage state change of ids. Hence, reentrancy is possible, as long as the token has the callback hook. For example, some ERC777 token, or even existing token could have this feature after upgrade.

// Line-of-Credit/contracts/modules/credit/LineOfCredit.sol
    function _close(Credit memory credit, bytes32 id) internal virtual returns (bool) {
        if(credit.principal > 0) { revert CloseFailedWithPrincipal(); }

        // return the Lender's funds that are being repaid
        if (credit.deposit + credit.interestRepaid > 0) {
            LineLib.sendOutTokenOrETH(
                credit.token,
                credit.lender,
                credit.deposit + credit.interestRepaid
            );
        }

        delete credits[id]; // gas refunds

        // remove from active list
        ids.removePosition(id);
        unchecked { --count; }

        // ...
    }

A malicious lender can call close(id) and reenter to drain the fund, or can use another address as the borrower to call depositAndClose() to do similar thing.

Tools Used

Manual analysis.

Recommended Mitigation Steps

For the _close() function:

• add nonReentrant modifier
• follow check-effects-interaction pattern

[c4-judge]

dmvt marked the issue as duplicate of #176

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[c4-judge]

dmvt marked the issue as satisfactory