code423n4 commented 2 years ago

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

GalloDaSballo commented 2 years ago

@dapp-whisperer wdyt?

jack-the-pug commented 2 years ago

Dup #9

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