hats-finance / Fenix-Finance-0x83dbe5aa378f3ce160ed084daf85f621289fb92f

0 stars 0 forks source link

Default rebasing YieldMode may break invariants #48

Open hats-bug-reporter[bot] opened 4 months ago

hats-bug-reporter[bot] commented 4 months ago

Github username: @zilayo Twitter username: -- Submission hash (on-chain): 0x74b2a565bdb8bb5552c8030e68c51c3051e3df9637fd1a276ed00b82efa3ed51 Severity: medium

Description: Description\ AlgebraPool contracts inherit from BlastERC20RebasingManage which allows claiming & configuring yield mode from the Blast rebasing ERC20s.

Blast's rebasing ERC20s have a few differences to standard ERC20, and careful consideration needs to be given - especially in protocols which rely heavily on accurate accounting.

The main differences are around balance calculations, and how transfer/transferFrom operates.

    function balanceOf(address account)
        public
        view
        virtual
        returns (uint256 value)
    {
        YieldMode yieldMode = _yieldMode[account];
        if (yieldMode == YieldMode.AUTOMATIC) {
            value = _computeShareValue(_shares[account], _remainders[account]);
        } else {
            value = _fixed[account];
        }
    }

// transfer / transferFrom ends up in _setBalance

    function _setBalance(address account, uint256 amount, bool resetClaimable) internal {
        uint256 newShares; uint256 newRemainder; uint256 newFixed;
        YieldMode yieldMode = getConfiguration(account);
        if (yieldMode == YieldMode.AUTOMATIC) {
            (newShares, newRemainder) = _computeSharesAndRemainder(amount);
        } else if (yieldMode == YieldMode.VOID) {
            newFixed = amount;
        } else if (yieldMode == YieldMode.CLAIMABLE) {
            newFixed = amount;
            uint256 shareValue = amount;
            if (!resetClaimable) {
                shareValue = _computeShareValue(_shares[account], _remainders[account]);
                shareValue = shareValue + amount - _fixed[account];
            }
            (newShares, newRemainder) = _computeSharesAndRemainder(shareValue);
        }

        _updateBalance(account, newShares, newRemainder, newFixed);
    }

Unless an account with the POOLS_ADMINISTRATOR role configures the YieldMode, all pools will be YieldAutomatic by default. This means that balanceOf, transfer, and transferFrom calls will utilize the shares and remainder values to calculate balances and amounts to transfer.

Since a YieldAutomatic account calculates amounts based on the underlying share price of the rebasing tokens, this may break certain invariants / assumptions that the contract has about it's own balances.

It is unreasonable for a POOLS_ADMINISTRATORaccount to manually configure each and every pool that's created, so ideally the YieldMode should be set during pool initialization.

In addition, the function to configure yield modes for the Algebra Pools don't have any safeguards against setting it to YieldAutomatic

function configure(address erc20Rebasing_, YieldMode mode_) external virtual returns (uint256) {
    _checkAccessForManageBlastERC20Rebasing();

    return IERC20Rebasing(erc20Rebasing_).configure(mode_);
  }

In the event that the YieldMode is misconfigured, this will also lead to the above issues.

Given that there is also functionality to claim accumulated yields, I assume that the intention is to have these pools be set to YieldClaimable.

It's important to note that attempts to claim yield will revert unless the contract is set to YieldClaimable:

 function claim(address recipient, uint256 amount) external returns (uint256) {
        address account = msg.sender;
        if (recipient == address(0)) {
            revert ClaimToZeroAddress();
        }

        if (getConfiguration(account) != YieldMode.CLAIMABLE) {
            revert NotClaimableAccount();
        }

Recommendations

  1. YieldMode should be set to either YieldClaimable or YieldVoid upon pool deployment. This will avoid any issues caused by balances changing, whilst remaining static within the pool's internal accounting.

    // BlastERC20RebasingManage.sol
    constructor() {
    IERC20Rebasing(0x4300000000000000000000000000000000000003).configure(YieldMode.CLAIMABLE); // USDB
    IERC20Rebasing(0x4300000000000000000000000000000000000004).configure(YieldMode.CLAIMABLE); // WETH
    }

    One option is to set the YieldMode for USDB & WETH in all pools to YieldClaimable by default. Alternatively logic could be placed elsewhere to configure only the relevant rebasing contract, depending on which tokens are in the pair.

  2. Safeguards should be placed to prevent the YieldMode from ever being set to Automatic:

    function configure(address erc20Rebasing_, YieldMode mode_) external virtual returns (uint256) {
        require(mode_ != YieldMode.AUTOMATIC, 'mode_ cannot be YieldMode.AUTOMATIC');
        _checkAccessForManageBlastERC20Rebasing();
    
        return IERC20Rebasing(erc20Rebasing_).configure(mode_);
    }
BohdanHrytsak commented 4 months ago

Thank you for the submission.

I wanted to see options in which this or that mode will really break something without assumptions.

AlgebraPool is basically able to work with rebasing tokens. Testing shows that AlgebraPool works as expected and correctly regardless of the mode of the Rebasing token.