Reentrancy in _close() allows single lender to steal all deposits from other lenders

[code423n4]

Lines of code

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

Vulnerability details

Impact

Upon calling close(), a lender's credit position is deleted AFTER the transfer out of their deposit. Therefore, an ERC777 will allow the lender to call close() again and receive the same amount of funds. The lender will be able to reenter the contract as many times as necessary to drain the token balance.

Proof of Concept

A lender or borrower can call close() which eventually calls _close(). The purpose is to return the lender's capital back to them and then delete the position. Since the position isn't deleted until after the transfer occurs, a token with a callback hook, such as an ERC777 or non-standard token like AMP will allow the lender to reenter.

    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

NOTE: In the current implementation, only tokens with callbacks are susceptible because the transfer of ETH is performed with transfer(). Wardens will flag this issue which might result in the DebtDAO team altering the implementation to call(). If/when they do, ETH transfers will be susceptible to reentrancy as well.

The bottom half of the _close() function does not contain any logic that would cause a revert.

        ids.removePosition(id);
        unchecked { --count; }


        // If all credit lines are closed the the overall Line of Credit facility is declared 'repaid'.
        if (count == 0) { _updateStatus(LineLib.STATUS.REPAID); }


        emit CloseCreditPosition(id);


        return true;
    }

• removePosition() does not revert if the position does not exist.
• Since --count is contained in an unchecked block, the UINT will underflow successfully.
• count will not equal true.

Tools Used

Manual review.

Recommended Mitigation Steps

Delete the credits[id] position prior to the transfer or add reentrancy guards.

[c4-judge]

dmvt marked the issue as duplicate of #160

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[c4-judge]

dmvt marked the issue as satisfactory