Skip to content
This repository has been archived by the owner on Apr 28, 2024. It is now read-only.

peanuts - Max collateral check is not done when increasing collateral balance #37

Closed
sherlock-admin opened this issue Oct 23, 2023 · 6 comments
Closed

peanuts - Max collateral check is not done when increasing collateral balance #37

sherlock-admin opened this issue Oct 23, 2023 · 6 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Non-Reward This issue will not receive a payout

Comments

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Oct 23, 2023 •
edited by Evert0x
Loading

peanuts

medium

Max collateral check is not done when increasing collateral balance

Summary

There is no max collateral check when increasing the collateral balance using increaseCollateralBalance()

Vulnerability Detail

When a user calls borrow(), there is a check for borrowingCollateral. The check makes sure that borrowingCollateral is not greater than maxCollateral

File: LiquidityBorrowingManager.sol
492:         uint256 borrowingCollateral = cache.borrowedAmount - cache.holdTokenBalance;
493:         (borrowingCollateral > params.maxCollateral).revertError(
494:             ErrLib.ErrorCode.TOO_BIG_COLLATERAL
495:         );

maxCollateral is defined as the maximum amount of collateral that can be provided for the loan.

File: LiquidityBorrowingManager.sol
35:         /// @notice The maximum amount of collateral that can be provided for the loan
36:         uint256 maxCollateral;

However, when increaseCollateralBalance is called, the maxCollateral variable is not checked.

File: LiquidityBorrowingManager.sol
371:     function increaseCollateralBalance(bytes32 borrowingKey, uint256 collateralAmt) external {
372:         BorrowingInfo storage borrowing = borrowingsInfo[borrowingKey];
373:         // Ensure that the borrowed position exists and the borrower is the message sender
374:         (borrowing.borrowedAmount == 0 || borrowing.borrower != address(msg.sender)).revertError(
375:             ErrLib.ErrorCode.INVALID_BORROWING_KEY
376:         );
377:         // Increase the daily rate collateral balance by the specified collateral amount
               //@audit -- No max collateral balance check
378:         borrowing.dailyRateCollateralBalance +=
379:             collateralAmt *
380:             Constants.COLLATERAL_BALANCE_PRECISION;
381:         _pay(borrowing.holdToken, msg.sender, VAULT_ADDRESS, collateralAmt);
382:         emit IncreaseCollateralBalance(msg.sender, borrowingKey, collateralAmt);
383:     }

Impact

Without checking maxCollateral, the leverage position may be unnecessarily overcollaterized.

Code Snippet

https://github.com/sherlock-audit/2023-10-real-wagmi/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L371-L383

Tool used

Manual Review

Recommendation

Recommend checking the max collateral amount when calling increase collateral balance
@github-actions github-actions bot added High A valid High severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Oct 26, 2023
@sherlock-admin sherlock-admin changed the title Restless Ocean Chipmunk - Max collateral check is not done when increasing collateral balance peanuts - Max collateral check is not done when increasing collateral balance Oct 30, 2023
@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Oct 30, 2023
@Ali-Shehab Ali-Shehab mentioned this issue Oct 30, 2023
Open
@seeques
Copy link

seeques commented Oct 31, 2023 •
edited
Loading

Escalate

This is not a duplciate of #86. Furthermore, it is invalid. maxCollateral represents a maximum amount of collateral a borrower wishes to give, and it is provided by the borrower in the borrow() function. There is no need for it in the increaseCollateralBalance() since in this case the borrower decides for how much collateral he wants to increase his position explicitly by passing the collateralAmt as the function argument.

@sherlock-admin2
Copy link
Contributor

sherlock-admin2 commented Oct 31, 2023 •
edited by sherlock-admin
Loading

Escalate

This is not a duplciate of #86. Furthermore, it is invalid. maxCollateral represents a maximum amount of collateral a borrower wishes to give, and it is provided by the borrower in the borrow() function. There is no need for it in the increaseCollateralBalance() since in this case the borrower decides for how much collateral he wants to increase his position explicitly by passing the collateralAmt as the function argument.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

@sherlock-admin sherlock-admin added the Escalated This issue contains a pending escalation label Oct 31, 2023
@nevillehuang
Copy link

Agree with the above comments. increaseCollateral() does not require an explicit max collateral ratio check, since there is no additional benefits of simply increasing collateral without borrowing.

@Czar102
Copy link

Czar102 commented Nov 4, 2023

Will be accepting the escalation and making this submission invalid.

@Evert0x Evert0x removed the High A valid High severity issue label Nov 7, 2023
@sherlock-admin sherlock-admin added Non-Reward This issue will not receive a payout and removed Reward A payout will be made for this issue labels Nov 7, 2023
@Evert0x
Copy link

Evert0x commented Nov 7, 2023

Result:
Invalid
Unique

@sherlock-admin2 sherlock-admin2 removed the Escalated This issue contains a pending escalation label Nov 7, 2023
@sherlock-admin2
Copy link
Contributor

Escalations have been resolved successfully!

Escalation status:

@sherlock-admin sherlock-admin added Escalation Resolved This issue's escalations have been approved/rejected and removed Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Nov 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Escalation Resolved This issue's escalations have been approved/rejected Non-Reward This issue will not receive a payout
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@Evert0x @Czar102 @nevillehuang @sherlock-admin @seeques @sherlock-admin2