Debt DAO contest - SmartSek's results

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/MutualConsent.sol#L38-L60

Vulnerability details

Impact

Neither lenders nor borrowers can revoke mutualConsents. This can pose an issue if a lender has given consent to lend funds but a borrower has proven untrustworthy either through actions in another credit line or some other public way. A lender should be able to revoke their mutual consent if they want to. In its current implementation, a lender can only "revoke" their consent by removing token allowance to the contract, though this seems like a workaround method and should be implemented natively.

Proof of Concept

_mutualConsent() does either of two actions:

1. Checks if the function data has been supplied by the counterparty. If so, executes the respective action.
2. If not, writes the consent to storage so that the counterparty can execute.

    function _mutualConsent(address _signerOne, address _signerTwo) internal returns(bool) {
        if(msg.sender != _signerOne && msg.sender != _signerTwo) { revert Unauthorized(); }


        address nonCaller = _getNonCaller(_signerOne, _signerTwo);


        // The consent hash is defined by the hash of the transaction call data and sender of msg,
        // which uniquely identifies the function, arguments, and sender.
        bytes32 expectedHash = keccak256(abi.encodePacked(msg.data, nonCaller));


        if (!mutualConsents[expectedHash]) {
            bytes32 newHash = keccak256(abi.encodePacked(msg.data, msg.sender));


            mutualConsents[newHash] = true;


            emit MutualConsentRegistered(newHash);


            return false;
        }


        delete mutualConsents[expectedHash];


        return true;
    }

Nowhere is the user able to revoke a previous consent.

Tools Used

Manual review.

Recommended Mitigation Steps

Allow a function to revoke a consent. A caller calling this function will delete the specified hash from the mutualConsents mapping.

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.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/LineLib.sol#L48

Vulnerability details

Impact

Note: I realize that this was flagged in the C4udit output, though it seems to believe that it's an ERC20 transfer.

The use of transfer() to send ETH only forwards 2300 gas to the recipient. This is not enough gas to execute a gnosis safe delegatecall. Funds can be lost in such a situation or similar edge cases.

Tools Used

Manual review.

Recommended Mitigation Steps

I recommend changing to .call() with fixed gas. I believe 30000 to be a nice amount with little risk of:

1. EIPs increasing op code gas costs exceeding the value.
2. Reentrancy.