code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L293 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L890 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L881 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L764

Vulnerability details

Impact

In StakedCitadel.sol/_mintSharesFor(), it doesn’t check whether the pool is equal to zero. If pool == 0 and totalSupply() != 0, _mintSharesFor() will revert. And In _depositFor(), it uses balance() as pool. In consequence, when the CTDL balance of StakedCitadel is zero and the total supply of xCTDL is not zero, _depositFor() always reverts. No one can ever deposit any CTDL.

Proof of Concept

Alice deposit 10 CTDLs into StakedCitadel, and get some xCTDL
Bob deposit 10 CTDLs into StakedCitadel, and get some xCTDL
AuthorizedActor calls earn(), and it transfer 95% of CTDL balance of StakedCitadel to strategy

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L717

    function earn() external {
        require(!pausedDeposit, "pausedDeposit"); // dev: deposits are paused, we don't earn as well
        _onlyAuthorizedActors();

        uint256 _bal = available();
        token.safeTransfer(strategy, _bal);
        IStrategy(strategy).earn();
    }

uint256 public constant MAX_BPS = 10_000
…
 toEarnBps = 9_500;
…
    function available() public view returns (uint256) {
        return (token.balanceOf(address(this)) * toEarnBps) / MAX_BPS;
    }

Bob calls withdrawAll(), It should take 10 CTDLs in this case. The CTDL balance of StakedCitadel is not enough after earn(), So it will transfer CTDLs from strategy to ensure r == token.balanceOf(address(this)). Since withdrawalFee is never higher than 2%, there is no fee in this transaction. In the end, the CTDL balance of StakedCitadel becomes zero.

    function _withdraw(uint256 _shares) internal nonReentrant {
        require(_shares != 0, "0 Shares");

        uint256 r = (balance() * _shares) / totalSupply();
        _burn(msg.sender, _shares);

        // Check balance
        uint256 b = token.balanceOf(address(this));
        if (b < r) {
            uint256 _toWithdraw = r - b;
            IStrategy(strategy).withdraw(_toWithdraw);
            uint256 _after = token.balanceOf(address(this));
            uint256 _diff = _after - b;
            if (_diff < _toWithdraw) {
                r = b + _diff;
            }
        }

        uint256 _fee = _calculateFee(r, withdrawalFee);
        uint256 _amount = r - _fee;

        // Send funds to vesting contract and setup vesting
        IVesting(vesting).setupVesting(msg.sender, _amount, block.timestamp);
        token.safeTransfer(vesting, _amount);

        // After you burned the shares, and you have sent the funds, adding here is equivalent to depositing
        // Process withdrawal fee
        if(_fee > 0) {
            _mintSharesFor(treasury, _fee, balance() - _fee);
        }
    }

Now balance() in _depositFor() returns zero, And totalSupply() in _mintSharesFor() returns non-zero, since Alice still has xCTDL. In consequence, _mintSharesFor() always reverts at shares = (_amount * totalSupply()) / _pool;, no one can deposit any CTDL into

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L764

   function _depositFor(address _recipient, uint256 _amount)
        internal
        nonReentrant
    {
        require(_recipient != address(0), "Address 0");
        require(_amount != 0, "Amount 0");
        require(!pausedDeposit, "pausedDeposit"); // dev: deposits are paused

        uint256 _pool = balance();
        uint256 _before = token.balanceOf(address(this));
        token.safeTransferFrom(msg.sender, address(this), _amount);
        uint256 _after = token.balanceOf(address(this));
        _mintSharesFor(_recipient, _after - _before, _pool);
    }

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L881

    function _mintSharesFor(
        address recipient,
        uint256 _amount,
        uint256 _pool
    ) internal {
        uint256 shares;
        if (totalSupply() == 0) {
            shares = _amount;
        } else {
            shares = (_amount * totalSupply()) / _pool;
        }
        _mint(recipient, shares);
    }

Tools Used

vim

Recommended Mitigation Steps

Add a check for pool in _mintSharesFor()

GalloDaSballo commented 2 years ago

The code is written in solidity 0.8.12 a division by zero will cause a revert.

For that reason I must disagree with the finding, I would like to see a code POC but I believe the POC would just revert per the reason above

jack-the-pug commented 2 years ago

I think this issue can be considered as a dup of #74

code-423n4 / 2022-04-badger-citadel-findings