search

code-423n4 / 2023-10-badger-findings

1 stars 1 forks source link

The `computeCR` function leads to users depositing more collateral than necessary, resulting in a loss of staking rewards. #281

Closed c4-submissions closed 9 months ago

c4-submissions commented 9 months ago

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/HintHelpers.sol#L207-L218

Vulnerability details

Impact

HintHelpers.sol includes three helper functions: getRedemptionHints, getApproxHint, and computeNominalCR. These functions help users in operating CDPs. However, the logic of the computeCR function is incorrect and will result in users depositing more collateral than necessary.

The computeCR function is designed to calculate ICR. This function takes three parameters: _coll, _debt, and _price. According to the comments, _coll represents The collateral amount in shares.

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/HintHelpers.sol#L207-L218

    /// @notice Compute CR for a specified collateral, debt amount, and price
    /// @param _coll The collateral amount, in shares
    /// @param _debt The debt amount
    /// @param _price The current price
    /// @return The computed CR for the given parameters
    function computeCR(
>>        uint256 _coll,
        uint256 _debt,
        uint256 _price
    ) external pure returns (uint256) {
        return EbtcMath._computeCR(_coll, _debt, _price);
    }

However, the first parameter of the _computeCR function is _stEthBalance, indicating the amount of stETH tokens. https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/Dependencies/EbtcMath.sol#L103-L108

    /// @dev Compute collateralization ratio, given stETH balance, price, and debt balance
    function _computeCR(
>>        uint256 _stEthBalance,
        uint256 _debt,
        uint256 _price
    ) internal pure returns (uint256) {

The keyword _coll are used in many functions and events. In all of those contexts, _coll consistently represents the collateral amount, in shares. Therefore, it can be confirmed that in the computeCR function, _coll indeed means the collateral amount in shares. Here are some scenarios where _coll is used: https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/Interfaces/ICdpManager.sol#L58-L64 https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/Interfaces/IActivePool.sol#L10-L13 https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/Dependencies/EbtcBase.sol#L83-L85

Therefore, the ICR calculated through the computeCR function is incorrect. If a user calls this function to estimate the required amount of stETH tokens before creating a CDP, they will get a value higher than the actual amount they should deposit. This results in two consequences:

(1) The user deposits more stETH tokens than necessary, and these excess tokens could have been used elsewhere. (2) stETH is Lido staking ETH, generating staking rewards. The eBTC project extracts 50% of the staking rewards as protocol fees. Hence, the user incurs a loss of 50% of the staking rewards. (The default value of stakingRewardSplit is 50%)

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/CdpManagerStorage.sol#L552-L558

    function calcFeeUponStakingReward(
        uint256 _newIndex,
        uint256 _prevIndex
    ) public view returns (uint256, uint256, uint256) {
        require(_newIndex > _prevIndex, "CDPManager: only take fee with bigger new index");
        uint256 deltaIndex = _newIndex - _prevIndex;
>>        uint256 deltaIndexFees = (deltaIndex * stakingRewardSplit) / MAX_REWARD_SPLIT;

Proof of Concept

Here is a test case. In this testing scenario, the value of eth per share, 1146574285570086126, is obtained from the stETH contract. Alice wants to borrow 1392750000000000000 eBTC tokens, aiming for an ICR of 160%. Upon calling the computeCR function, it is found that when the value of _coll is 30000000000000000000, the ICR is precisely 160%. Subsequently, the stETH token quantity is calculated through stEthBalance = collateral.getPooledEthByShares(coll), and finally, the opencdp function is invoked to create the CDP.

    function testComputeCR() public {
        bytes32  dummyId = 0x0000000000000000000000000000000000000000000000000000000000000000;
        address payable[] memory users;
        users = _utils.createUsers(1);
        address Alice = users[0];

        _dealCollateralAndPrepForUse(Alice);  
        collateral.setEthPerShare(1146574285570086126);

        vm.startPrank(Alice);
        uint256 debt = 1392750000000000000;
        uint256 coll = 30000000000000000000;
        uint256 icr = hintHelpers.computeCR(coll, debt, priceFeedMock.fetchPrice());    // 1600000000000000000
        uint256 stEthBalance = collateral.getPooledEthByShares(coll);
        bytes32 cdpid = borrowerOperations.openCdp(debt, dummyId, dummyId, stEthBalance + 0.2 ether);
        vm.stopPrank();

        console.log("stETH expected: %s", coll);
        console.log("stETH actually: %s", stEthBalance);
        console.log("ICR expected: %s", icr);
        console.log("ICR actually: %s", cdpManager.getSyncedICR(cdpid, priceFeedMock.fetchPrice()));
    }

The test result shows that the actual ICR value is 183.4%. Alice has deposited an excess of 4397228567102583780 stETH tokens. Additionally, Alice will incur a loss of 50% of the staking rewards generated by these tokens.

Running 1 test for foundry_test/BorrowerOperations.openCloseCdp.t.sol:OpenCloseCdpTest
[PASS] testComputeCR() (gas: 626766)
Logs:
  stETH expected: 30000000000000000000
  stETH actually: 34397228567102583780
  ICR expected: 1600000000000000000
  ICR actually: 1834518856912137801

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.91ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Foundry

Recommended Mitigation Steps

Calculate the amount of stETH balance based on _coll and then proceed to calculate the ICR.

    function computeCR(
        uint256 _coll,
        uint256 _debt,
        uint256 _price
    ) external pure returns (uint256) {
        return EbtcMath._computeCR(collateral.getPooledEthByShares(_coll), _debt, _price);
    }

Assessed type

Context

bytes032 commented 9 months ago

the collateral amount is already in shares

c4-pre-sort commented 9 months ago

bytes032 marked the issue as insufficient quality report

GalloDaSballo commented 9 months ago

Seems invalid, cannot understand this

The function uses shares

jhsagd76 commented 9 months ago

https://github.com/code-423n4/2023-10-badger/blob/2bacb215bf3257320e4f6dd7dcb039f1f06f3214/packages/contracts/foundry_test/PositionManagers.t.sol#L204-L208

to

https://github.com/code-423n4/2023-10-badger/blob/2bacb215bf3257320e4f6dd7dcb039f1f06f3214/packages/contracts/foundry_test/BaseFixture.sol#L571-L574

invalid

c4-judge commented 9 months ago

jhsagd76 marked the issue as unsatisfactory: Invalid

nianyanchuan commented 9 months ago

https://github.com/code-423n4/2023-10-badger/blob/2bacb215bf3257320e4f6dd7dcb039f1f06f3214/packages/contracts/foundry_test/PositionManagers.t.sol#L204-L208

to

https://github.com/code-423n4/2023-10-badger/blob/2bacb215bf3257320e4f6dd7dcb039f1f06f3214/packages/contracts/foundry_test/BaseFixture.sol#L571-L574

invalid

The first parameter of computeCR function is misleading. It's named _coll, and the NatSpec documentation states @param _coll The collateral amount, in shares. I've categorized this bug as medium risk because the computeCR function yields an incorrect CR. The accurate computation should be stEthBalance * price / debt. Therefore, return EbtcMath._computeCR(_coll, _debt, _price); should be changed to return EbtcMath._computeCR(collateral.getPooledEthByShares(_coll), _debt, _price);. It's evident that the return value of _getCdpStEthBalance represents the collateral amount in steth balance, not in shares, so the result of that test case is right.

A simpler approach might be to directly modify the parameter and NatSpec, changing _coll to _stEthBalance and @param _coll The collateral amount, in shares to @param _stEthBalance The collateral amount, in stETH balance.

jhsagd76 commented 9 months ago

Can't understand whats your point. You mean that the param name is ambiguous, so it should be a MED?

Negative, based on rules speculated from future code.

nianyanchuan commented 9 months ago

I didn't find this test case when submitting the report. If the first parameter is meant to represent the collateral amount in shares, then the return value of this function is indeed incorrect. That's why I categorized it as a medium-risk issue when submitting the report. Based on this test case and the C4 rules, your judgment is more reasonable. Lastly, it would be advisable for the Badger team to modify the name of this parameter and updates the NatSpec accordingly.