search

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

1 stars 1 forks source link

Lack of logic to reflect a negative rebase of Lido (stETH), which lead to that the CollSurplusPool would suffer the additional loss #251

Closed c4-submissions closed 9 months ago

c4-submissions commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/CollSurplusPool.sol#L80-L81 https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/CollSurplusPool.sol#L91 https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/CollSurplusPool.sol#L107

Vulnerability details

Description

When a redemption, the CdpManager#_closeCdpByRedemption() would be called via the CdpManager#_redeemCollateralFromCdp() like this: https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/CdpManager.sol#L171-L177

Within the CdpManager#_closeCdpByRedemption(), the CollSurplusPool#increaseSurplusCollShares() would be called like this: https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/CdpManager.sol#L256-L257

    /*
     * Called when a full redemption occurs, and closes the cdp.
     * The redeemer swaps (debt) EBTC for (debt)
     * worth of stETH, so the stETH liquidation reserve is all that remains.
     * In order to close the cdp, the stETH liquidation reserve is returned to the Cdp owner,
     * The debt recorded on the cdp's struct is zero'd elswhere, in _closeCdp.
     * Any surplus stETH left in the cdp, is sent to the Coll surplus pool, and can be later claimed by the borrower.
     */
    function _closeCdpByRedemption(
        bytes32 _cdpId,
        uint256 _EBTC,
        uint256 _collSurplus,
        uint256 _liquidatorRewardShares,
        address _borrower
    ) internal {
        ...
        // Update Active Pool EBTC, and send ETH to account
        activePool.decreaseSystemDebt(_EBTC);

        // Register stETH surplus from upcoming transfers of stETH collateral and liquidator reward shares
        collSurplusPool.increaseSurplusCollShares(_borrower, _collSurplus + _liquidatorRewardShares); ///<-------- @audit
        ...

Within the CollSurplusPool#increaseSurplusCollShares(), balances[_account] + _amount of surplus collateral (stETH) would be stored as the newAmount into the balances[_account] like this: https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/CollSurplusPool.sol#L80-L81

    /// @notice Increase collateral surplus balance for owner _account by _amount
    /// @param _account The owner address whose surplus balance is increased
    /// @param _amount The surplus increase value
    /// @dev only CdpManager is allowed to call this function
    function increaseSurplusCollShares(address _account, uint256 _amount) external override {
        _requireCallerIsCdpManager();

        uint256 newAmount = balances[_account] + _amount; ///<-----------@audit
        balances[_account] = newAmount;  ///<-----------@audit

        emit SurplusCollSharesUpdated(_account, newAmount);
    }

After that, when a borrower claim their surplus collateral (stETH), the CollSurplusPool#claimSurplusCollShares() would be called via the BorrowerOperations#claimSurplusCollShares() like this: https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/BorrowerOperations.sol#L601

Within the CollSurplusPool#claimSurplusCollShares(), the balances[_account] would be stored into the claimableColl. Then, the amount (claimableColl) of collateral (stETH) would be transferred to the given _account like this: https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/CollSurplusPool.sol#L91 https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/CollSurplusPool.sol#L107

    /// @notice Allow owner to claim all its surplus recorded in this pool
    /// @dev stETH token will be sent to _account address if any surplus exist
    /// @param _account The owner address whose surplus balance is to be claimed
    function claimSurplusCollShares(address _account) external override {
        _requireCallerIsBorrowerOperations();
        uint256 claimableColl = balances[_account]; ///<---------- @audit 
        ...

        balances[_account] = 0;  ///<---------- @audit 
        emit SurplusCollSharesUpdated(_account, 0);

        ...
        emit CollSharesTransferred(_account, claimableColl);

        // NOTE: No need for safe transfer if the collateral asset is standard. Make sure this is the case!
        collateral.transferShares(_account, claimableColl); ///<---------- @audit - stETH (Collateral) would be transferred to the given "_account"
    }

According to the past report by OpenZeppelin, there is a case that a rebase of Lido (stETH) would be a negative direction due to slashing event like this:

the underlying asset stETH is able to rebase in both a positive and negative direction due to the potential for slashing.

Based on that, it is supposed to reflect a negative rebase of Lido (stETH) to the claimable surplus collateral (balance[_account]) and cover the loss due to it in the CollSurplusPool if the negative rebase of Lido would occur.

However, within the CollSurplusPool#claimSurplusCollShares(), there is no logic to reflect a negative rebase of Lido to the claimable collateral (balance[_account]) and cover the loss due to it if the negative rebase of Lido would occur.

Note
At the moment, within the CollSurplusPool#claimSurplusCollShares(), the balance of surplus collateral (stETH) of the borrower (balance[_account]), which was recorded when a redemption via the CdpManager#_closeCdpByRedemption(), would be used as it is when the borrower would claim their surplus collateral via the CollSurplusPool#claimSurplusCollShares() (via the BorrowerOperations#claimSurplusCollShares()).

This is problematic. Because the CollSurplusPool would transfer the exact same amount (balance[_account]) of collateral (stETH) as before a negative rebase.

Impact

The CollSurplusPool will have additional losses in that case. Because, despite a negative rebase would decreases the cost of stETH share and therefore the surplus collateral (stETH) balance of the CollSurplusPool would be decreased, the CollSurplusPool would still attempt to transfer the exact same amount of surplus collateral (stETH) as before a negative rebase when a borrower would claim their surplus collateral. This lead to transferring more amount of surplus collateral (stETH) than the amount that should be transferred.

Proof of Concept

Here is a possible scenario:

Note Lido's rebase is a daily rebase, meaning that the rebase would occur every single day.

Tools Used

Recommended Mitigation Steps

Within the CollSurplusPool#claimSurplusCollShares(), consider adding a logic that reflect a negative rebase of Lido to the claimable surplus collateral (stETH) and cover the loss due to it if a negative rebase of Lido would occur.

Assessed type

Other

c4-pre-sort commented 10 months ago

bytes032 marked the issue as sufficient quality report

GalloDaSballo commented 10 months ago

All math is in shares, this looks invalid

c4-sponsor commented 10 months ago

GalloDaSballo (sponsor) disputed

jhsagd76 commented 9 months ago

agree with sponsor, I believe this report has confused the distinction between "balance" and "scaledBalance", I suggest providing a complete poc for verification.

c4-judge commented 9 months ago

jhsagd76 marked the issue as unsatisfactory: Insufficient proof