2024-06-gondi-updated-draft.md

sponsor	slug	date	title	findings	contest
Gondi	2024-06-gondi	2024-07-25	Gondi Invitational	https://github.com/code-423n4/2024-06-gondi-findings/issues	394

Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Gondi smart contract system written in Solidity. The audit took place between June 14 — July 5, 2024.

Wardens

In Code4rena's Invitational audits, the competition is limited to a small group of wardens; for this audit, 2 wardens contributed reports:

1. oakcobalt
2. minhquanym

This audit was judged by 0xsomeone.

Final report assembled by thebrittfactor.

Summary

The C4 analysis yielded an aggregated total of 1 unique vulnerabilities. Of these vulnerabilities, 0 received a risk rating in the category of HIGH severity and 1 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 2 reports detailing issues with a risk rating of LOW severity or non-critical.

Note: findings related to unreleased pools have been redacted as that feature is currently under embargo.

Scope

The code under review was composed of 29 smart contracts written in the Solidity programming language and includes 4117 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

Medium Risk Findings (1)

[M-01] Delegations cannot be removed in some cases due to vulnerable revokeDelegate() implementation

Submitted by oakcobalt

An old borrower can use an old delegation to claim on behalf of a new borrower.

Proof of Concept

A borrower can delegate locked collateral NFT through delegateRegistry to prove token ownership and claim airdrops, event ticketing, etc.

delegateRegistry by Delegate.Cash protocol allows custom rights to be configured to a delegatee.

Currently, MultiSourceLoan::delegate allows a borrower to configure bytes32 _rights to delegateERC721. In DelegateRegistry::delegateERC721, bytes32 rights will be hashed as part of the key to store delegation data.

//src/lib/loans/MultiSourceLoan.sol
    function delegate(uint256 _loanId, Loan calldata loan, address _delegate, bytes32 _rights, bool _value) external {
        if (loan.hash() != _loans[_loanId]) {
            revert InvalidLoanError(_loanId);
        }
        if (msg.sender != loan.borrower) {
            revert InvalidCallerError();
        }
        //@audit-info a borrower can pass custom rights to delegateERC721
|>      IDelegateRegistry(getDelegateRegistry).delegateERC721(
            _delegate, loan.nftCollateralAddress, loan.nftCollateralTokenId, _rights, _value
        );

        emit Delegated(_loanId, _delegate, _value);
    }

src/lib/loans/MultiSourceLoan.sol#L484

The problem is, in MultiSourceLoan::revokeDelegate, empty rights will always be passed to delegateERC721. This means when a borrower configures custom rights in delegate(), they cannot remove the delegation. In DelegateRegistry::delegateERC721, empty rights will be hashed into a different key from the borrower's actual delegation. Incorrect delegation data will be read and delegateERC721 call will return with no change.

//src/lib/loans/MultiSourceLoan.sol
    function revokeDelegate(address _delegate, address _collection, uint256 _tokenId) external {
        if (ERC721(_collection).ownerOf(_tokenId) == address(this)) {
            revert InvalidMethodError();
        }
        //@audit revokeDelegate will always pass empty rights.
|>      IDelegateRegistry(getDelegateRegistry).delegateERC721(_delegate, _collection, _tokenId, "", false);

        emit RevokeDelegate(_delegate, _collection, _tokenId);
    }

src/lib/loans/MultiSourceLoan.sol#L496

POC:

1. The original borrower set custom rights and delegated their collateral NFT to a custom contract.
2. The original borrower's loan ended and NFT is transferred to a new borrower.
3. The protocol or the new borrower calls revokeDelegate() to remove previous delegations of the NFT.
4. The new borrower takes out a loan with the NFT and calls delegate(), delegating the NFT to a hot wallet.
5. The original borrower's old delegation is not cleared from delegateRegistry and still claims an event ticket using the old delegation. The old borrower claims the new borrower's ticket.

Recommended Mitigation Steps

In revokeDelegate(), allow passing bytes32 _rights into delegateERC721() to correctly revoke existing delegations with custom rights.

Assessed type

Error

0xend (Gondi) confirmed

0xsomeone (judge) commented:

The Warden has outlined how the protocol will incorrectly integrate with the DelegateRegistry system, attempting to revoke a previous delegation via an empty payload which is a futile attempt as proper revocation would require the same _rights to be passed in with a false value for the _enable flag.

I am slightly mixed in relation to this submission as the MultiSourceLoan::delegate function can be utilized with a correct payload to remove delegation from the previous user correctly. I believe that users, protocols, etc., will attempt to use the MultiSourceLoan::revokeDelegate function to revoke their delegation, and thus, a medium-risk severity rating is appropriate even though a circumvention already exists in the code.

To note, the code also goes against its interface specification (src/interfaces/loans/IMultiSourceLoan.sol#L257-L261) further re-inforcing a medium-risk rating level.

---

Low Risk and Non-Critical Issues

For this audit, 2 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by oakcobalt received the top score from the judge.

The following wardens also submitted reports: minhquanym.

[L-01] Delegated and RevokeDelegate event should have bytes32 _rights

Instances (2)

When a borrower delegate or revokeDelegate, custom delegation rights bytes32 _rights will not be included in event Delegated and event RevokeDelegate.

In DelegateRegistry, custom rights bytes32 _rights are hashed into delegation key when storing delegation data. bytes32 _rights is crucial in tracking delegations off-chain.

//src/lib/loans/MultiSourceLoan.sol
    function delegate(
        uint256 _loanId,
        Loan calldata loan,
        address _delegate,
        bytes32 _rights,
        bool _value
    ) external {
...
        IDelegateRegistry(getDelegateRegistry).delegateERC721(
            _delegate,
            loan.nftCollateralAddress,
            loan.nftCollateralTokenId,
            _rights,
            _value
        );
|>      emit Delegated(_loanId, _delegate, _value);

src/lib/loans/MultiSourceLoan.sol#L487

//src/lib/loans/MultiSourceLoan.sol
    function revokeDelegate(address _delegate, address _collection, uint256 _tokenId) external {
...
|>        emit RevokeDelegate(_delegate, _collection, _tokenId);

src/lib/loans/MultiSourceLoan.sol#L498

Recommendations

Consider adding bytes32 _rights field in Delegated and RevokeDelegate event.

[L-02] _checkStrictlyBetter might underflow revert without triggering the custom error

In MutliSourceLoan::refinanceFull(), if lender initiated the refinance, _checkStrictlyBetter() will run to ensure if the lender provided more principal; the annual interest with the new principal is still better than existing loan. If it's not strictly better, the tx should throw with a custom error NotStrictlyImprovedError(). However, the custom error might not be triggered due to an earlier underflow error.

If _offerPrincipalAmount < _loanPrincipalAmount, or if _loanAprBps * _loanPrincipalAmount < _offerAprBps * _offerPrincipalAmount, the tx will throw without triggering the custom error.

//src/lib/loans/MultiSourceLoan.sol
    function _checkStrictlyBetter(
        uint256 _offerPrincipalAmount,
        uint256 _loanPrincipalAmount,
        uint256 _offerEndTime,
        uint256 _loanEndTime,
        uint256 _offerAprBps,
        uint256 _loanAprBps,
        uint256 _offerFee
    ) internal view {
...
        if (
|>           ((_offerPrincipalAmount - _loanPrincipalAmount != 0) &&
|>                ((_loanAprBps *
                    _loanPrincipalAmount -
                    _offerAprBps *
                    _offerPrincipalAmount).mulDivDown(
                        _PRECISION,
                        _loanAprBps * _loanPrincipalAmount
                    ) < minImprovementApr)) ||
            (_offerFee != 0) ||
            (_offerEndTime < _loanEndTime)
        ) {
            revert NotStrictlyImprovedError();
        }
...

src/lib/loans/MultiSourceLoan.sol#L1112-L1116

Recommendations

Check for underflow and revert with custom error early.

[L-03] Unnecessary math operation when _remainingNewLender is set to type(uint256).max in the refinance flow

When a lender initiates refinanceFull() or refinancePartial(), _remainingNewLender will be set to type(uint256).max (src/lib/loans/MultiSourceLoan.sol#L280), which indicates the lender will repay existing lenders.

However, even when _remainingNewLender is set to type(uint256).max, in _processOldTranche(), _remainingNewLender -= oldLenderDebt will still run. This is not meaningful.

//src/lib/loans/MultiSourceLoan.sol
    function _processOldTranche(
        address _lender,
        address _borrower,
        address _principalAddress,
        Tranche memory _tranche,
        uint256 _endTime,
        uint256 _protocolFeeFraction,
        uint256 _remainingNewLender
    ) private returns (uint256 accruedInterest, uint256 thisProtocolFee, uint256 remainingNewLender) {
...
        if (oldLenderDebt > 0) {
            if (_lender != _tranche.lender) {
                asset.safeTransferFrom(_lender, _tranche.lender, oldLenderDebt);
            }
            unchecked {
|>              _remainingNewLender -= oldLenderDebt;
            }
        }
|>       remainingNewLender = _remainingNewLender;

src/lib/loans/MultiSourceLoan.sol#L665

Recommendations

In _processOldTranche(), add a check and only perform the subtraction when _remainingNewLender != type(uint256).max.

[L-04] Incorrect spelling

There's one instance of incorrect spelling: renegotiationIf should be renegotiationId.

//src/lib/loans/BaseLoan.sol
    mapping(address user => mapping(uint256 renegotiationIf => bool notActive))
        public isRenegotiationOfferCancelled;

src/lib/loans/BaseLoan.sol#L57

Recommendation

Change renegotiationIf into renegotiationId.

[L-05] Borrower can use an arbitrary offerId for a pool contract's loan offer, which might lead to incorrect off-chain accounting.

offerId is a field in struct LoanOffer (src/interfaces/loans/IMultiSourceLoan.sol#L26) and LoanOffer is typically signed by the lender. Currently, offerId is generated off-chain and its correctness is verified through _checkSignature(lender, offer.hash(), _lenderOfferSignature (src/lib/loans/MultiSourceLoan.sol#L780) in the emitLoan flow.

But when the lender is a pool contract, offerId will not be verified due to _checkSignature() being bypassed in _validateOfferExectuion() (src/lib/loans/MultiSourceLoan.sol#L777).

A borrower can supply any offerIds for a pool lender offer in emitLoan() or refinanceFromLoanExeuctionData() flow. As a result, emit LoanEmitted(loanId, offerIds, loan, totalFee) or emit LoanRefinancedFromNewOffers(_loanId, newLoanId, loan, offerIds, totalFee) will contain arbitrary offerIds. This may create conflicts in off-chain offerId accounting.

Recommendation

If offerId for a pool lender is relevant, consider allowing a pool contract to increment and store its next offerId on-chain.

[L-06] Borrowers might use a lender's addNewTranche renegotiation offer to refinanceFull in some cases

A borrower can use a lender’s renegotiation offer signature for addNewTranch() in refinanceFull(), as long as the loan only has one tranche.

This is because refinanceFull() only checks whether _renegotiationOffer.trancheIndex.length == _loan.tranche.length (src/lib/loans/MultiSourceLoan.sol#L168). When there's only one tranche in the loan, addNewTranche()'s renegoationOffer's check condition (src/lib/loans/MultiSourceLoan.sol#L379) will also satisfy.

refinanceFull() will also ensure the refinanced tranche gets a better apr (src/lib/loans/MultiSourceLoan.sol#L591). So the borrower gets better apr for the existing tranche instead of taking out additional principal in addNewTranche().

In addition, if the lender signed a renegotiation offer intended for refinanceFull(), the same offer can also be used for addNewTranche() if the condition _renegotiationOffer.trancheIndex[0] == _loan.tranche.length is satisfied. Because _renegotiationOffer.trancheIndex[0] is never checked in refinanceFull() flow, the lender might supply any values. In this case, the lender is forced to open a more junior tranche which can be risky for lenders.

It's better to prevent the same renegotiation offer from being used interchangeably in different methods with different behaviors.

Recommendation

In refinanceFull(), add a check to ensure _renegotiationOffer.trancheIndex[0]==0.

[L-07] Consider adding a cap for minLockPeriod

_minLockPeriod is used to compute the lock time for a tranche or a loan. If the ratio is set too high (e.g., 10000), tranches or loans cannot be refinanced due to failing _loanLocked() checks (src/lib/loans/MultiSourceLoan.sol#L181).

//src/lib/loans/MultiSourceLoan.sol
    function setMinLockPeriod(uint256 __minLockPeriod) external onlyOwner {

        _minLockPeriod = __minLockPeriod;

        emit MinLockPeriodUpdated(__minLockPeriod);
    }

src/lib/loans/MultiSourceLoan.sol#L508

Recommendation

Considering adding a cap value (e.g., 10%).

[L-08] updateLiquidationContract() might lock collaterals and funds in the current liquidator contract

In LiquidationHandler::updateLiquidationContract(), the loan liquidator contract can be updated.

    function updateLiquidationContract(address __loanLiquidator) external override onlyOwner {
        __loanLiquidator.checkNotZero();
        _loanLiquidator = __loanLiquidator;
        emit LiquidationContractUpdated(__loanLiquidator);
    }

src/lib/LiquidationHandler.sol#L74

There are two vulnerable conditions: updateLiquidationContract() is called when there are ongoing/unsettled auctions in the current liquidator or there might be a pending liquidateLoan() tx.

1. If MultisourceLoan's liquidator contract is updated. None of the exiting auctions originated from the MultisourceLoan can be settled because AuctionLoanLiquidator::settleAuction will call IMultiSourceLoan(_auction.loanAddress).loanLiquidated(... (src/lib/AuctionLoanLiquidator.sol#L300). This will cause the onlyLiquidator modifier to revert (src/lib/loans/MultiSourceLoan.sol#L464). MultiSourceLoan contract no longer recognizes the old liquidator contract. The collateral and bid funds will be locked in the old liquidator contract.

2. If there is a pending MultiSourceLoan::liquidateLoan (src/lib/loans/MultiSourceLoan.sol#L445) tx before updateLiquidationContract() call. The auction of the loan will be created right before updateLiquidationContract() settles. Similar to number 1, the collateral will be locked in the old liquidator contract. In addition, since MultiSourceLoan::liquidateLoan() is permissionless, an attacker can front-run updateLiquidationContract tx to cause the loan liquidated to an old liquidator contract.

Recommendation

1. In UpdateLiquidationContract(), consider adding a check that the existing liquidator’s token balance is 0, with no outstanding auction.
2. Only update liquidationContract when there are no liquidatable loans.

[L-09] Unnecessary code - BytesLib methods are not used in this contract or its parent contracts

BytesLib methods are not used in PurchaseBundler.sol or its parent contracts.

//src/lib/callbacks/PurchaseBundler.sol
    using BytesLib for bytes;
...

src/lib/callbacks/PurchaseBundler.sol#L24

Recommendation

Consider removing this line.

[L-10] Some proposed callers might not be confirmed

LoanManagerParameterSetter.sol has a two-step process of adding callers. The issue is addCallers() doesn't check whether _callers.length == proposedCallers.length. If _callers.length < proposedCaller.length, some proposedCallers' indexes will not run in the for-loop. proposedCallers whose indexes are after callers will not be added as callers.

//src/lib/loans/LoanManagerParameterSetter.sol
    function addCallers(ILoanManager.ProposedCaller[] calldata _callers) external onlyOwner {
        if (getProposedAcceptedCallersSetTime + UPDATE_WAITING_TIME > block.timestamp) {
            revert TooSoonError();
        }
        ILoanManager.ProposedCaller[] memory proposedCallers = getProposedAcceptedCallers;
        uint256 totalCallers = _callers.length;
|>      for (uint256 i = 0; i < totalCallers;) {
            ILoanManager.ProposedCaller calldata caller = _callers[i];
            if (
                proposedCallers[i].caller != caller.caller || proposedCallers[i].isLoanContract != caller.isLoanContract
            ) {
                revert InvalidInputError();
            }

            unchecked {
                ++i;
            }
        }
        ILoanManager(getLoanManager).addCallers(_callers);
    }

src/lib/loans/LoanManagerParameterSetter.sol#L110

Recommendation

Add check to ensure _callers.length == proposedCallers.length.

[L-11] Incorrect comments

Instances (2)

1. Auction Loan liquidator -> User Vault

/// @title Auction Loan Liquidator
/// @author Florida St
/// @notice NFTs that represent bundles.
contract UserVault is ERC721, ERC721TokenReceiver, IUserVault, Owned {
...

src/lib/UserVault.sol#L13

2. address(0) = ETH -> address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE) = ETH

    /// @notice ERC20 balances for a given vault: token => (vaultId => amount). address(0) = ETH
    mapping(address token => mapping(uint256 vaultId => uint256 amount)) _vaultERC20s;

src/lib/UserVault.sol#L33

Recommendation

Correct comments.

[L-12] vaultID’s NFT/ERC20 bundle can be modified while the loan is outstanding

UserVault.sol allows a user to bundle assets (NFT/ERC20) together in a vault to be used as a collateral NFT.

According to doc, the intended behavior is new NFTs cannot be added to the vault unless borrower burn the vault and create a new vaultId with a new bundle of asset.

This is not currently the case in UserVault.sol. Anyone can deposit ERC20 or ERC721 to an existing vaultID at any time. Although this doesn’t decrease assets from the vault, this may increase VaultID assets at any time during lender offer signing, loan outstanding, and loan liquidation auction process.

    function depositERC721(uint256 _vaultId, address _collection, uint256 _tokenId) external {
        _vaultExists(_vaultId);

        if (!_collectionManager.isWhitelisted(_collection)) {
            revert CollectionNotWhitelistedError();
        }
        _depositERC721(msg.sender, _vaultId, _collection, _tokenId);
    }

    function _depositERC721(address _depositor, uint256 _vaultId, address _collection, uint256 _tokenId) private {
        ERC721(_collection).transferFrom(_depositor, address(this), _tokenId);

        _vaultERC721s[_collection][_tokenId] = _vaultId;

        emit ERC721Deposited(_vaultId, _collection, _tokenId);
    }

src/lib/UserVault.sol#L152-L158

Increasing the assets of a vaultId doesn’t put a loan’s collateralization at risk. However, this may create inconsistencies in lender offers due to vaultId‘s changing asset bundle.

Due to the permissionless deposit process of UserVault.sol, this may also allow a malicious actor to deposit assets to a vaultID during auciton to manipulate bidding.

Recommendation

If the intention is to disallow adding new NFTs to a vault before burning of vaultId, consider a two-step deposit and vault mint process: caller deposit assets to a new vaultId first before minting the vaultId and disallow deposit to a vaultId after minting.

---

Disclosures

C4 is an open organization governed by participants in the community.

C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.