Badger Citadel contest - rayn's results

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()

Lines of code

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

Vulnerability details

Impact

When doing withdraw() or withdrawAll() in StakedCitadel.sol, it will call the internal function _withdraw(). The function then transfers tokens to the vesting contract, which should be StakedCitadelVester.sol. However, IVesting(vesting).setupVesting() is not implemented in StakedCitadelVester.sol. This mistake makes the withdraw functions totally unusable.

Proof of Concept

In StakedCitadel.sol/_withdraw(), it uses IVesting(vesting).setupVesting(msg.sender, _amount, block.timestamp); to setup vesting.

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

    function _withdraw(uint256 _shares) internal nonReentrant {
        …

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

        …
    }

However in StakedCitadelVester.sol. The setup function is actually vest(). setupVesting() is not implemented in StakedCitadelVester.sol

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

    /**
     * @dev setup vesting for recipient.
     * @notice note that a given address can only have one active vest at a time.
     * @notice adding a new vest before claiming completely from the previous will re-lock the previous amount according to the new vesting timeline.
     * @param recipient The account for which vesting will be setup.
     * @param _amount amount that will be vested
     * @param _unlockBegin The time at which unlocking of tokens will begin.
     */
    function vest(
        address recipient,
        uint256 _amount,
        uint256 _unlockBegin
    ) external {
        require(msg.sender == vault, "StakedCitadelVester: only xCTDL vault");
        require(_amount > 0, "StakedCitadelVester: cannot vest 0");

        vesting[recipient].lockedAmounts =
            vesting[recipient].lockedAmounts +
            _amount;
        vesting[recipient].unlockBegin = _unlockBegin;
        vesting[recipient].unlockEnd = _unlockBegin + vestingDuration;

        emit Vest(
            recipient,
            vesting[recipient].lockedAmounts,
            _unlockBegin,
            vesting[recipient].unlockEnd
        );
    }

Tools Used

vim

Recommended Mitigation Steps

Either

replace IVesting(vesting).setupVesting with IVesting(vesting).vest in StakedCitadel.sol

or

replace vest with setupVesting in StakedCitadelVester.sol

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L132 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L85 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadelVester.sol#L108

Vulnerability details

Impact

When users vest in StakedCitadelVester, the tokens will be locked in vest duration. Users can call claim() to get back their tokens. The claimable amount is calculated in claimableBalance(). Before the duration ends, the claimable amount is

((locked * (block.timestamp - vesting[recipient].unlockBegin)) /
                (vesting[recipient].unlockEnd -
                    vesting[recipient].unlockBegin)) - claimed

It seems to be a fair claimable amount. However if users keep vesting more and more, the claimableBalance() will become unfair and sometimes revert.

Proof of Concept

• Alice vests 10000 CTDL, then vesting[recipient].lockedAmounts becomes 10000

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

    function vest(
        address recipient,
        uint256 _amount,
        uint256 _unlockBegin
    ) external {
        …
        vesting[recipient].lockedAmounts =
            vesting[recipient].lockedAmounts +
            _amount;
        vesting[recipient].unlockBegin = _unlockBegin;
        vesting[recipient].unlockEnd = _unlockBegin + vestingDuration;
        …
    }

• Alice claim 10000 CTDL after the duration ends, then vesting[msg.sender].claimedAmounts becomes 10000

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

    function claim(address recipient, uint256 amount) external {
        uint256 claimable = claimableBalance(msg.sender);
        if (amount > claimable) {
            amount = claimable;
        }
        if (amount != 0) {
            vesting[msg.sender].claimedAmounts =
                vesting[msg.sender].claimedAmounts +
                amount;
            vestingToken.safeTransfer(recipient, amount);
            emit Claimed(msg.sender, recipient, amount);
        }
    }

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

    function claimableBalance(address recipient) public view returns (uint256) {
        uint256 locked = vesting[recipient].lockedAmounts;
        uint256 claimed = vesting[recipient].claimedAmounts;
        if (block.timestamp >= vesting[recipient].unlockEnd) {
            return locked - claimed;
        }
        return
            ((locked * (block.timestamp - vesting[recipient].unlockBegin)) /
                (vesting[recipient].unlockEnd -
                    vesting[recipient].unlockBegin)) - claimed;
    }

• Alice then vest 100 CTDL, vesting[recipient].lockedAmounts becomes 10100

    function vest(
        address recipient,
        uint256 _amount,
        uint256 _unlockBegin
    ) external {
        …
        vesting[recipient].lockedAmounts =
            vesting[recipient].lockedAmounts +
            _amount;
        …
    }

• After half of vesting duration, Alice wants to claim some token. But it will revert in claimableBalance()

Since

locked = vesting[recipient].lockedAmounts = 10100
claimed = vesting[recipient].claimedAmounts = 10000

 ((locked * (block.timestamp - vesting[recipient].unlockBegin)) /
                (vesting[recipient].unlockEnd -
                    vesting[recipient].unlockBegin))   =   10100 * (1/2) = 5050

 ((locked * (block.timestamp - vesting[recipient].unlockBegin)) /
                (vesting[recipient].unlockEnd -
                    vesting[recipient].unlockBegin)) - claimed = 5050 - 10000

This line will revert

        return
            ((locked * (block.timestamp - vesting[recipient].unlockBegin)) /
                (vesting[recipient].unlockEnd -
                    vesting[recipient].unlockBegin)) - claimed;

• Alice can still claim all the tokens after the vesting duration ends, but as Alice keeps vesting, it gets harder to claim tokens back during vesting duration. Since lockedAmounts will keep growing.

Tools Used

vim

Recommended Mitigation Steps

vest() could be modified like the following code.

    function vest(
        address recipient,
        uint256 _amount,
        uint256 _unlockBegin
    ) external {
        require(msg.sender == vault, "StakedCitadelVester: only xCTDL vault");
        require(_amount > 0, "StakedCitadelVester: cannot vest 0");

        vesting[recipient].lockedAmounts =
            vesting[recipient].lockedAmounts +
            _amount -
            vesting[recipient].claimedAmounts ;
        vesting[recipient].claimedAmounts = 0;
        vesting[recipient].unlockBegin = _unlockBegin;
        vesting[recipient].unlockEnd = _unlockBegin + vestingDuration;

        emit Vest(
            recipient,
            vesting[recipient].lockedAmounts,
            _unlockBegin,
            vesting[recipient].unlockEnd
        );
    }

Summary

We list 3 low-critical findings here:

• (Low) transferFromDisabled is unused
• (Low) No check for _daoId
• (Low) setXXXXXLimits, setMaxXXXXX and setXXXXXBounds functions don’t consider the original value

In conclusion, it's better to check unused variables, check the validation of DAO ID, and check the boundaries of original values.

(Low) transferFromDisabled is unused

Impact

transferFromDisabled is unused and not initialized, it may cause some problems when we need it in the future.

Proof of Concept

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

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Delete unused variables or set transferFromDisabled in initialize().

(Low) No check for _daoId

Impact

No check for _daoId of buy() function in KnightingRound.sol, resulting in a user being able to buy non-exist or invalid _daoId.

Proof of Concept

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

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Check valid range of _daoId in buy() function.

(Low) setXXXXXLimits, setMaxXXXXX and setXXXXXBounds functions don’t consider the original value

Impact

In set limit function (e.g. setDiscountLimits), it doesn’t consider the original value (funding.discount), resulting in the original value being out of bound.

Proof of Concept

For example, the original value funding.discount is 20, minDiscount is 10, and maxDiscount is 100. If Governance calls setDiscountLimits, changes minDiscount to 30, and changes maxDiscount to 50, the original funding.discount will be out of bound.

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L356 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L397 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L521 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L533 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L548

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Check the original value in set limit functions.

Save gas in for loops by unchecked arithmetic

The for loop has no overflow risk of i. Use an unchecked block to save gas.

Proof of Concept

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/lib/GlobalAccessControlManaged.sol#L48 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L111 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/SupplySchedule.sol#L208 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L152 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L344

Recommendation

Use unchecked blocks to avoid overflow checks, or use ++i rather than i++ if you don't use unchecked blocks.

for (uint256 i = 0; i < length; ) {
    ...
    unchecked {
        ++i;
    }
}