Goldilocks Goldilend

When a user repays a loan completely, the collateral NFT is returned and the loan is marked as repaid. However, renew() does not check if the loan has been repaid, allowing users to receive new funds without providing any collateral.

A user can repay a loan completely with repay() in both BeraBondGoldilend and RebaseGoldilend contracts. When the loan is fully repaid, the collateral NFT is transferred back to the user and repaid is set to true.

However, renew() does not verify if the loan has been repaid or if the collateral still exists, allowing users to exploit this vulnerability.

The exploit works as follows:

1. User borrows funds with NFT collateral
2. User repays the loan completely receiving back the NFT
3. User calls renew() on the same loanId
4. User receives new funds without providing any collateral
5. Repeat

This also apply for liquidation flow.

Critical. Attacker can steal all funds by renewing already repaid or liquidated loans without providing any collateral.

Verify in renew() that userLoan.repaid == false and userLoan.liquidated == false.

Fixed in commit 32980ec965b9a3e23d3631adf34c227f1e766066.

The repay() function allows repayment of loans that have already been repaid, enabling attackers to repeatedly withdraw their NFT collateral and drain protocol funds.

The repay() function in both RebaseGoldilend and BeraBondGoldilend lacks validation to prevent repayment of already repaid loans.

The function caps the repayment amount to the borrowed amount:

if(repayAmount > userLoan.borrowedAmount) repayAmount = userLoan.borrowedAmount;

For already repaid loans where borrowedAmount is 0, this sets repayAmount to 0. The function then marks the loan as repaid and returns the NFT collateral, regardless of whether the loan was previously repaid.

An attacker can exploit this by:

1. Taking a loan and repaying it normally
2. Taking a new loan
3. Calling repay() with the old loan ID (and 1 wei payment to bypass the if(msg.value == 0) revert InvalidAmount() check in BeraBondGoldilend version)
4. Receiving the NFT collateral back for essentially free
5. Repeating steps 2-4 until all protocol funds are drained

Critical. Attackers can steal all protocol funds by repeatedly exploiting already repaid loans.

Add a check to prevent repayment of already repaid and liquidated loans.

Fixed in commit ceac8609a462989fee645eb030f542fc1b01ea90.

When a user borrows assets, the contract calculates and charges interest upfront, but it is incorrectly deducted upfront and sent to multisig. This results in higher borrow rates than expected.

Users can borrow assets with borrow() in both BeraBondGoldilend and RebaseGoldilend contracts.

The issue occurs because:

1. Interest is calculated on the full borrowAmount requested
2. Interest is deducted upfront and sent to multisig
3. User receives only borrowAmount - interest

This creates a discrepancy where the effective interest rate paid by users is higher than the nominal rate, as shown in the following example:

• User requests: 1000 BERA
• Interest calculated: 100 BERA (10% of 1000)
• User receives: 900 BERA (1000 - 100)
• Effective rate: 100/900 = 11.11% instead of 10%

Medium. Users pay higher effective interest rates than expected, especially problematic for high rates or long durations. The contract also incorrectly tracks pool utilization including interests.

Transfer all the borrowAmount to the user and add the interest to it in storage. Check that borrowAmount + interest is not above the NFT fair value is already in place.

Logic and checks involving borrowAmount should also be changed accordingly if needed.

Acknowledged.

The renew() function allows users to extend loans beyond the intended maxDuration limit by calling the function multiple times consecutively.

In both BeraBondGoldilend and RebaseGoldilend, the renew() function validates that newDuration falls within the minDuration and maxDuration bounds. However, it then adds this duration to the existing loan:

newUserLoan.endDate += newDuration

This allows users to batch multiple renew() calls, each adding up to maxDuration to their loan, effectively creating loans that last much longer than intended. A user could potentially extend their loan indefinitely by repeatedly calling renew() with valid duration parameters.

Medium. Users can bypass the maximum duration restriction, creating liquidity issues for depositors and circumventing protocol-intended loan limits.

Modify the renew() function to set the loan duration from the current timestamp rather than extending it:

newUserLoan.endDate = block.timestamp + newDuration

This ensures loans never exceed the maximum allowed duration.

Fixed in commit b74bb183a2eb902d9ce9a3e8adc77d7015752b98 and a2705e9e4e0df7944abbe8107854470a84f799e5.

The claimYield() function allows users to claim rewards from NFTs they previously used as collateral but no longer own, as it fails to verify the loan status.

When borrowing in in the BeraBondGoldilend and contract, the NFT ID used is added to userTokenIds.

When loans are repaid or liquidated in the BeraBondGoldilend contract, the NFT ID is not deleted from the userTokenIdsis not deleted. Instead, it remains in storage and the loan struct is updated with liquidated or repaid set to true and borrowedAmount set to 0.

The claimYield() function allows users to claim rewards on Token Bound Accounts (TBAs) linked to BeraBonds used as collateral. However, it does not verify whether the associated loans are still active:

This creates a scenario where:

1. User A borrows against a BeraBond NFT
2. User A repays the loan and retrieves the NFT
3. User A transfers the NFT to User B
4. User B uses the same NFT as collateral for a new loan
5. User A can still call claimYield() to claim rewards from the NFT now owned by User B

Additionally borrow() will always push the NFT ID to the userTokenIds no matter if it as already been added in the past, leading to potential duplicated of IDs inside the array.

Medium. Previous NFT holders can steal yield from current NFT owners who use the same ID.

Delete the NFT ID from the userTokenIds array when repaying or liquidating a loan.

Fixed in commit 00b5051aa34a8e82e9136af1a990d2423b6e3747 and 3a0e566db27f6ef69fe3b48f54473772e5f36a92.

The increaseglDebtAssetBacking() function instantly increases share value, allowing attackers to sandwich the transaction and extract value intended for historical lenders.

The function increaseglDebtAssetBacking() can be called by the multisig to increase the share value of the debtAsset token. This function is typically called after liquidations when NFT collateral is sent to the multisig during liquidate() calls, and the team recovers proceeds from liquidating the collateral or using insurance funds.

The share value increase is applied instantly but at a later time than the liquidation, creating two issues:

1. Sandwich attacks: An attacker can deposit large amounts just before the multisig calls increaseglDebtAssetBacking(), then immediately withdraw after the transaction to capture most of the value increase at the expense of historical lenders.

2. Historical lender dilution: Even without sandwich attacks, new depositors that deposited just after the liquidation but before the call to increaseglDebtAssetBacking() benefit from the share increase intended to reimburse historical lenders for liquidation losses, diluting the compensation meant for those who suffered the original losses.

Medium. The share value increase mechanism can be exploited through sandwich attacks and inherently dilutes compensation for historical lenders who suffered liquidation losses.

Consider implementing a streaming mechanism to distribute the backing increase over time, or restrict deposits/withdrawals during the backing increase process. Alternatively, implement a separate compensation mechanism that directly benefits affected lenders without diluting their recovery through new deposits.

Fixed in commits fa8dbaf5e95732b62fde5cc23f83ce644198c381, 1c3480ea42f6f47987310b48d51977a570e28ba0, 5fa6ec612908c143c628212c2a65547bc7c7c092, a73a57400c63248cbac6684c9f7eb45ed1d739cb and bdfaba489b74cb0fda9b0344beeea45b4a27c20a.

Users can repeatedly renew loans with short durations to pay significantly less interest than borrowing for the full intended duration upfront.

The function _calculateInterest() calculates interest with an exponential duration and amount weight - longer duration especially, result in higher interest rates.

Users can exploit this by taking short-term loans and repeatedly calling renew() with short durations instead of borrowing for the full intended period.

Example calculation:

Parameters: rate = 20e18, debt = 100e18, borrowAmount = 50e18, poolSize = 200e18, slope = 2e18

//Single 100-day loan:
_calculateInterest(borrowAmount, debt, 100 days) //→ 4.6e18 interest

//Ten 10-day renewals:
_calculateInterest(borrowAmount, debt, 10 days) //→ 0.29e18 per renewal
//Total for 100 days: 0.29e18 × 10 = 2.9e18 interest

//Savings: 4.6e18 - 2.9e18 = 1.7e18 (37% reduction)

Users can automate this process with bots to continuously renew loans and minimize interest payments.

Medium. Users can exploit the interest calculation mechanism to pay significantly lower rates than intended, reducing protocol revenue and creating unfair advantages for sophisticated users.

Implement a fixed renewal fee or modify the interest calculation to account for cumulative loan duration, preventing users from gaming the system through frequent renewals.

Acknowledged, we believe this is not an issue. First, we have a minimum duration check so there is a minimum length that the loan will always be on renewals, and users will always have to pay the interest on that duration up front. And it’s okay if a user gets a lower rate by repeatedly renewing compared to doing one large loan, because the protocol is then taking on less risk because it is able to update the valuations of the collateral in between renewals.

The borrow() and renew() functions lack slippage protection, potentially causing users to receive less funds than expected due to changing interest rates.

The functions borrow() and renew() in both RebaseGoldilend and BeraBondGoldilend do not include slippage parameters.

Interest is paid upfront and can fluctuate based on:

• Current utilization percentage
• Protocol parameters that can be updated by the team

Between transaction submission and execution, interest calculations may change significantly, causing users to receive less borrowed funds than anticipated. Users may only want to proceed if their net borrowing amount meets a minimum threshold.

Medium. Users may receive less borrowed funds than expected due to interest rate changes between transaction preparation, submission and execution.

Add a minOut parameter to both functions and validate that the amount sent to users meets their minimum requirements.

Fixed in commit ad3b6f15f8dc9a64e60c8c99c4e10d7ebd1771f5.

• _calculateInterest() calculates interest based on the total borrowedAmount but the last check against fairValue doesn't account for existing interest that has already been paid upfront as it does in borrow().
• _calculateInterest() doesn't check for maxUtilization as it does in borrow().

Low. This can lead to incorrect information passed to function caller.

Modify _calculateInterest() in RebaseGoldilend as follow:

-       if(borrowAmount > fairValue || borrowAmount > poolSize - debt) revert BorrowLimitExceeded();
-       return _calculateInterest(borrowAmount, debt, duration);
+       uint256 _interest = _calculateInterest(borrowAmount, debt, duration);
+       if(borrowAmount + interest > fairValue || borrowAmount > poolSize - debt) revert BorrowLimitExceeded();
+       if(debt + borrowAmount > poolSize * maxUtilization / 100) revert MaxUtilizationExceeded();
+       return _interest;

Fixed in commit f1b650fe146b22e53740f7d105298ae74eacbd59.

The repay() function blocks repayment after the grace period expires, while renew() allows loan extensions at any time, creating inconsistent behavior.

In both BeraBondGoldilend and RebaseGoldilend contracts, the repay() function includes a check that prevents repayment once block.timestamp > userLoan.endDate + LOAN_GRACE_PERIOD:

if(block.timestamp > userLoan.endDate + LOAN_GRACE_PERIOD) revert LoanExpired();

However, the renew() function lacks this same validation, allowing users to extend loans indefinitely regardless of how far past the deadline they are.

This inconsistency enables users to bypass the repayment deadline restriction by:

1. Waiting until after the grace period expires (when repay() would revert)
2. Calling renew() with minimal parameters (short duration, small amount)
3. Immediately calling repay() since the loan deadline has been reset

Low. Users can circumvent deadline restrictions through loan renewal, undermining the intended grace period enforcement.

Add the same deadline check to renew() for consistency:

if(block.timestamp > userLoan.endDate + LOAN_GRACE_PERIOD) revert LoanExpired();

Alternatively, remove the check from repay() to allow both functions to operate without deadline restrictions.

Fixed in commit 59408f41680c11fa244296a60552f102e427b9fe.

The renew() function only checks the new borrow amount against the pool size limit, allowing users to exceed the 10% borrowing restriction through multiple renewals.

The renew() function in both BeraBondGoldilend and RebaseGoldilend includes a check to prevent borrowing more than 10% of the pool size:

if(newBorrowAmount > _poolSize / 10) revert InvalidLoanAmount();

However, this check only considers the newBorrowAmount parameter and ignores the user's existing borrowedAmount. This allows users to bypass the borrowing limit by calling renew() multiple times with amounts just under the 10% threshold, accumulating a total borrowed amount that exceeds the intended limit.

Low. The pool size borrowing limit can be easily circumvented, undermining the protocol's risk management controls.

Modify the check to include the existing borrowed amount:

if(userLoan.borrowedAmount + newBorrowAmount > _poolSize / 10) revert InvalidLoanAmount();

Fixed in commit 302a158c767e1afe68de86aeb816652adff9318e.

The claimYield() function loops through all user NFT IDs, including those from repaid or liquidated loans, causing unnecessary gas consumption and potential transaction failures.

The function claimYield() always iterates through the entire userTokenIds array.

This can be an issue as the array currently doesn't see its NFT IDs removed from it when a user repays his loan or gets liquidated. Additionally, a user may have yield available for only one NFT ID and not all of them, trying to claim on all of them will cost extra gas and might even revert.

Low. Inefficient gas usage and potential denial of service for users with many historical loans.

Add parameters to allow partial claiming:

function claimYield(address[] memory rewardContracts, uint256 startIndex, uint256 endIndex) external {
    // Only process userTokenIds[startIndex:endIndex]
}

This enables users to claim yield for a subset of NFT IDs instead of all at once.

Fixed in commit 1786177e49e0fab0ef602fa02d22e78c4a4ba77d and 00b5051aa34a8e82e9136af1a990d2423b6e3747.

Liquidations can lower the poolSize down all the way to zero. When depositing, the function _glDebtAssetMintAmount() returns the amount of shares depending on the current ratio between supply and poolSize, unless one of the two is 0 in which case it will mint 1:1 shares per asset.

When withdrawing, it acts differently - if the poolSize is 0 then it will revert with a division by zero. This shouldn't be a problem as the shares are worth zero so there is no real interest for a user to withdraw 0 assets.

However there is an issue for depositors. If the poolSize is 0 then they will mint 1:1 shares, increasing the backing for already existing shares which will dilute the new depositors.

Example:

• 100 shares exist, poolSize is 0 due to liquidations, the ratio is 0
• New user deposits 100 assets, receives 100 shares
• Now there are 200 shares for 100 assets so the ratio moved from 0 to 0.5
• Older users that experienced liquidations can withdraw up to 50 assets at the cost of that new lender

Low. It's unlikely that poolSize goes all the way down to 0 and ideally the team is able to reimburse liquidations in a timely manner, but an unaware user may end up depositing and getting diluted.

Consider preventing deposits when poolSize == 0 but supply > 0

Fixed in commit 801a80be3a1fc6305d0596a593a742209a70a5c6.

There is inconsistent behavior between RebaseGoldilend and BeraBondGoldilend, some checks are done differently which can lead to unexpected behavior.

For example inside repay(), inside the RebaseGoldilend the repayAmount is bounded to outstandingDebt while inside the BeraBondGoldilend it is bounded to userLoan.borrowedAmount.

This inconsistency may lead to different repayment calculations and behaviors between the two contracts, potentially confusing users and creating integration issues.

Low. While it doesn't seem to be creating major issues, this could lead to unexpected behavior and assumptions between the two contracts.

Standardize the logic between both contracts to ensure consistent behavior.

Consider using a base contract and inheriting from it as suggested in https://github.com/electisec/goldilocks-goldilend-report/issues/4.

Partially fixed in commit 0949d4b8ceeac999307e075577874bd8453d7867. Lack of Base contract can still lead to inconsistencies.

When a user borrows assets, the contract creates a loan with ID userLoansLength + 1 but stores the NFT ID in userTokenIds[msg.sender].push(collateralNFTId). This creates a mismatch between loan IDs and their corresponding NFT positions in the array.

Informational.

Fix the mismatch by either using the same ID for both the loan and the NFT position.

Acknowledged.

Both BeraBondGoldilend and RebaseGoldilend contracts contain identical implementations for internal functions, variables and some checks, leading to code duplication. While the main functions (borrow, repay, renew, liquidate) have similar logic but handle different collateral types, the internal mathematical and utility functions are completely duplicated.

This duplication means:

• Security fixes for internal functions must be applied to both contracts
• Bug fixes risk being missed in one of the contracts
• Future development should be done twice with error risk

Informational.

Refactor the common code into a base contract that both BeraBondGoldilend and RebaseGoldilend can inherit from.

Acknowledged, thank you for this issue. These contracts previously did inherit from the same base contract but I thought due to the native token debt asset and collateral specific functions in BeraBondGoldilend that it made separate contracts necessary.

• Both BeraBondGoldilend and RebaseGoldilend contracts declare a multisigClaims state variable that is never used.
• In RebaseGoldilend the state variable timelock is never used.

Informational.

Remove the variable from both contracts or use them.

Fixed in commit 63a94a3c4d05115ba52978c5f3a950cdcee4e7fc.

The contract uses unchecked blocks in loops which are no longer necessary from Solidity version 0.8.22 as the compiler automatically handles loop variable increments optimization.

Informational.

Remove unchecked blocks in loops. This will improve code readability.

Fixed in commit 981ca4f4ac519b9c789063f7dc463f2ed12e4904.

Both BeraBondGoldilend and RebaseGoldilend contracts implement onERC721Received() which is unnecessary as the contracts only needs to receive NFTs as collateral during borrow(). This theoretically allows users to accidentally send NFTs to the contract even though the use of safeTranferFrom(), potentially resulting in locked assets.

Informational.

Remove the onERC721Received() function entirely.

Fixed in commit 5efe6fe80f72f7edfe36f7ad460cec076980454f.

initializeBeras() and initializeParameters() contains duplicated code as the same logic already exists in changeValue() and changeLendingParams().

Informational.

Refactor initializeBeras() and initializeParameters(), eliminating the duplicated logic:

function initializeParameters(
    uint256 _protocolInterestRate,
    uint256 _minDuration,
    uint256 _maxDuration,
    uint256 _slope,
    uint256 _maxUtilization
) external {
    if (msg.sender != multisig) revert NotMultisig();
    if (parametersInitialized) revert AlreadyInitialized();

    setParameters(_protocolInterestRate, _minDuration, _maxDuration, _slope, _maxUtilization);

    parametersInitialized = true;
}

function initializeBeras(address[] calldata _nfts, uint256[] calldata _nftFairValues) external {
    if (msg.sender != multisig) revert NotMultisig();
    if (berasInitialized) revert AlreadyInitialized();

    changeValue(_nfts, _nftFairValues);

    berasInitialized = true;
    borrowingActive = true;
}

setParameters() and changeValue() must be made public.

Fixed in commit 6e764b7ae95d33a55956c2f2dcc73db6e3e4e2f0.

The glDebtAsset tokens are minted when users deposit assets. However, when interests are accrued, these funds are sent directly to the multisig instead of being added to the pool. This means that while glDebtAsset tokens represent pool shares, they do not accumulate value over time as users would expect from a vault-like system.

Informational. Devs state that all revenue/interest goes to a multisig that then bribes a Berachain reward vault for the lending receipt token.

Clearly document the whole process.

Acknowledged, this process will be outlined in the documentation, UI, and marketing materials.

_calculateInterest() uses an unnecessary division by 100 at the end, which can be eliminated by properly setting the protocolInterestRate in 1e18 precision format instead of the current 1e20 format.

Informational.

Change the initialization of protocolInterestRate with a 1e18 precision format value, remove the /100 division at the end of _calculateInterest. This will make the interest calculation more transparent.

Fixed in commit a976a5f3c7515762ea14dc73a9689043b39d77ad.

The repay() function in both BeraBondGoldilend and RebaseGoldilend allows users to send more assets than their outstanding loan balance.

While the function caps the repayment amount:

if(repayAmount > userLoan.borrowedAmount) repayAmount = userLoan.borrowedAmount;

The excess payment is neither refunded to the user nor used to increase the debtAsset backing, resulting in permanent loss of the overpaid assets.

Informational. Users who accidentally overpay lose their excess funds with no recovery mechanism.

Consider reverting when the payment exceeds the loan's borrowedAmount to prevent accidental asset loss.

Fixed in commit fff7ad21293a8c8ea542dedcde5b828e25003e46.

The contract uses safeTransferFrom() to transfer NFTs during liquidation, but since the contract is already the owner of the NFT, received as collateral, using transfer() would be more gas efficient. safeTransfer() performs additional checks, verifying if the recipient is a contract by calling onERC721Received(), which are unnecessary as the multisig is a protocol known contract.

Gas savings.

Replace safeTransferFrom() with transfer() in the liquidation function since the contract is already the owner of the NFT.

Fixed in commit b32554a839dee30012476659371288fd58159b3b.

The liquidate() function in BeraBondGoldilend and RebaseGoldilend checks both liquidated and borrowedAmount == 0 to determine if a loan is valid for liquidation:

if((block.timestamp < userLoan.endDate + LOAN_GRACE_PERIOD || userLoan.liquidated || userLoan.borrowedAmount == 0) revert InvalidLoan();

When a loan is repaid or liquidated, the borrowedAmount is set to 0. Since borrowedAmount == 0 already identifies loans that cannot be liquidated, checking userLoan.liquidated is redundant and wastes gas.

Gas savings.

Simplify the check by removing the redundant liquidated flag verification:

if((block.timestamp < userLoan.endDate + LOAN_GRACE_PERIOD || userLoan.borrowedAmount == 0) revert InvalidLoan();

Fixed in commit 71fe0f7dfc8cb153ff013818389af52cc472258e.