Badger Citadel contest - 0xDjango's results

Lines of code

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

Vulnerability details

Impact

In StakedCitadel.sol, the withdraw() function calls the vesting contract to create a vesting schedule. The StakedCitadel.sol contract calls a function called setupVesting() as defined by IVesting.sol, however, the StakedCitadelVester.sol contract contains a function named vest() instead.

This bug removes all users' ability to withdraw from StakedCitadel.sol.

Proof of Concept

StakedCitadel.sol makes a call to the Vesting contract via:

IVesting(vesting).setupVesting(msg.sender, _amount, block.timestamp);

IVesting.sol complies with this function definition via:

interface IVesting {
    function setupVesting(
        address recipient,
        uint256 _amount,
        uint256 _unlockBegin
    ) external;
}

The vesting contract StakedCitadelVester.sol DOES NOT comply with this function definition as the function is named vest(), containing the same parameters.

    function vest(
        address recipient,
        uint256 _amount,
        uint256 _unlockBegin
    ) external {

Tools Used

Manual review.

Recommended Mitigation Steps

Reviewing BaseFixture.sol, it appears the vesting functionality is not tested, probably due to the wait time required by the vesting schedule. The Interface and call to the interface from StakedCitadel.sol need to be updated to use the vest() function OR the StakedCitadelVester.sol vest() function needs to be renamed to setupVesting().

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L887-L892

Vulnerability details

Impact

An early depositor to StakedCitadel.sol, preferably the first to deposit, will have the ability to steal funds from subsequent user deposits. The malicious user is able to do this by directly transferring tokens to the StakedCitadel.sol contract following their deposit. The direct token transfer will inflate the price for a single xCitadel share. If subsequent user deposits are less than the amount of token sent directly to the contracts, they will lose funds because their tokens will successfully transfer to the contract while they will be minted 0 shares due to precision loss.

Proof of Concept

Steps to exploit:

• Malicious User A deposits 1 wei of token. User A receives 1 wei of share since supply is currently 0 based on the following clause.

        if (totalSupply() == 0) {
            shares = _amount;
        }

• Malicious User A then directly sends token to StakedCitadel.sol which will inflate the balance of the contract. For this example, User A sends 10,000 ether of token to the contract.
• The next user B attempts to deposit 5,000 ether of token. Based on the following formula, they will receive 0 shares even though their funds transfer to the contract successfully.

      } else {
          shares = (_amount * totalSupply()) / _pool;
      }
      _mint(recipient, shares);

Where _pool is equal to balance() which is equal to token.balanceOf(address(this)). The formula for the example looks like:

shares = (5,000 ether * 1 wei of share) / 10,000 ether = 0 shares

• Malicious User A then calls withdraw(). Since the contracts hold 15,000 ether of tokens and User A holds the only share, they receive all 15,000 ether.

Tools Used

Manual review.

Recommended Mitigation Steps

Instead of relying on the balance of tokens of the two contracts, use internal accounting variables to represent the current balance of tokens that have been properly deposited and withdrawn from the vault. This will remove the ability to directly transfer tokens to the contracts to alter the share distribution logic

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L202-L216

Vulnerability details

Impact

The deposit() function of Funding.sol calls getAmountOut() to determine the amount of citadel the user receives in exchange for an asset. The getAmountOut() function contains a bug that results in the return being 0 if funding.discount is set to 0.

In the initializer, the FundingParams struct has funding.discount set to 0 so it is my understanding that deposits while there is no discount should be allowed.

funding = FundingParams(0, 0, 0, address(0), 0, _assetCap);

Where the first parameter in the struct is the discount. The issue in getAmountOut() can be seen below. If discount == 0, then the final return statement will always be 0 because citadelAmount_ is not defined at that point.

    function getAmountOut(uint256 _assetAmountIn)
        public
        view
        returns (uint256 citadelAmount_)
    {
        uint256 citadelAmountWithoutDiscount = _assetAmountIn * citadelPriceInAsset;


        if (funding.discount > 0) {
            citadelAmount_ =
                (citadelAmountWithoutDiscount * MAX_BPS) /
                (MAX_BPS - funding.discount);
        }


        citadelAmount_ = citadelAmount_ / assetDecimalsNormalizationValue;
    }

Proof of Concept

I have recreated the function and necessary variables in Remix and can verify that all deposits when discount equals 0 result in 0 citadel returned. It is important to note that the line xCitadel.depositFor(msg.sender, citadelAmount_); will fail with a citadelAmount_ of 0, so this bug simply causes a lack of protocol functionality and not loss of user funds. For a quick POC of the return value when discount equals 0, please refer to the following code which can quickly be run in Remix:

// SPDX-License-Identifier: GPL-3.0

pragma solidity 0.8.12;

contract GetAmountsOutTest {

    uint private citadelPriceInAsset = 2500;
    uint private MAX_BPS = 10000;
    uint private assetDecimalsNormalizationValue  = 10**8;
    
    struct FundingParams {
        uint256 discount;
        uint256 minDiscount;
        uint256 maxDiscount;
        address discountManager;
        uint256 assetCumulativeFunded; /// persistent sum of asset amount in over lifetime of contract.
        uint256 assetCap; /// Max asset token that can be taken in by the contract (defines the cap for citadel sold)
    }

    FundingParams private funding;

    function setFunding(uint256 _discount) public {
        funding = FundingParams(_discount, 0, 0, address(0), 0, 10000);
    }

    function getAmountOut(uint256 _assetAmountIn)
            public
            view
            returns (uint256 citadelAmount_)
        {
            uint256 citadelAmountWithoutDiscount = _assetAmountIn * citadelPriceInAsset;

            if (funding.discount > 0) {
                citadelAmount_ =
                    (citadelAmountWithoutDiscount * MAX_BPS) /
                    (MAX_BPS - funding.discount);
            }

            citadelAmount_ = citadelAmount_ / assetDecimalsNormalizationValue;
        }

}

Tools Used

Remix

Recommended Mitigation Steps

Adding the following else block would fix the bug:

        if (funding.discount > 0) {
            citadelAmount_ =
                (citadelAmountWithoutDiscount * MAX_BPS) /
                (MAX_BPS - funding.discount);
        } else {
            citadelAmount_ = citadelAmountWithoutDiscount;
       }

Therefore citadelAmount_ will now be non-zero both when there is a discount and when there isn't.

Lines of code

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

Vulnerability details

Impact

After attempting a withdrawal, StakedCitadelVester.sol vest() is called, creating a 21 day vesting schedule for the user to claim their withdrawed amount. This logic works perfectly for the first withdrawal, but will be incorrect for every subsequent withdrawal.

High level, the issue is because of the order of operations in claimableBalance(). The vested amount that the user is able to claim is calculated via:

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

It is important to note that locked and claimed are cumulative amounts for the entire history of the user's withdrawals. They do not reset after any calls to claim or vest.

Take a scenario where a user withdraws 100 ether of token. They wait for their 21 day vesting period to end, then they claim it all. Later, they attempt to withdraw 5 ether of token. In this example, locked = 105 ether and claimed = 100 ether. Therefore, the user won't be able to claim any of their newly-vested withdrawal until they have vested AT LEAST 100 ether. Doing the math, the user wouldn't have any claimableBalance until the very last day of the vesting period.

Proof of Concept

A simple example:

• User withdraws 21 ether of token.
• locked = 21 ether and claimed = 0 ether
• User waits all 21 day vesting period to claim.
• locked = 21 ether and claimed = 21 ether

Time passes...

• User withdraws another 21 ether of token.
• locked = 42 ether and claimed = 21 ether

While the proper linear vesting schedule would resemble an increase of 1 ether every day, their true vesting schedule looks as follows (skipping some days for space). NOTE: a negative value would revert, so equivalates to 0.

Day | Claimable Amount 0 | -21 1 | -19 2 | -17 3 | -15 ............. 9 | -3 10 | -1 11 | 1 12 | 3 ............ 20 | 19 21 | 21

To summarize, it isn't until the 11th day of the vesting schedule that they have a positive claimable amount. By day 21, they are able to claim the entirety of the withdrawed amount. If a user attempts multiple withdraws over time, this vesting schedule will resemble more of a 21 day freeze of funds.

Tools Used

Manual review

Recommended Mitigation Steps

The issue with the calculation is that the claimed amount is always being subtracted at the end. If claimed is larger than the amount vested by the day, the claimable amount is 0. Moving the subtraction of claimed to the beginning would fix the issue:

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

Issue 1 (Low) - Starting time check should be inclusive

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/SupplySchedule.sol#L184

The require statement should include the globalStartTimestamp. > should be changed to >=

Issue 2 (Low) - Front-runnable initializers

There are several initialize() functions that can be frontrun, requiring redeployment. Consider adding simple access control to these functions.

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelToken.sol#L22-L29

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/SupplySchedule.sol#L43-L46

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/SupplySchedule.sol#L43-L46

Issue 3 (Low)- Floating Pragma

A single contract contains a floating pragma. It is recommended to deploy all contracts with a single, specific compiler version to reduce the risk of compiler-specific bugs and contracts deployed with different versions. In the case of the forked contacts, I recommend deploying with the exact version that the current live versions were deployed with.

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelToken.sol#L2

Issue 4 (Low) - Code Consistency: Use of Days vs Seconds

In one contract, Days is used. In another 86400 seconds is used. For consistency, consider switching to a single method.

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadelVester.sol#L34

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/SupplySchedule.sol#L25

Issue 5 (Low)- Unnecessary parameter

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/KnightingRound.sol#L209

The tokenOutAmount_ parameter is not used. Instead it is always set in the function.

Issue 6 (Low) - CEI: Move state changes to after token transfer

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/KnightingRound.sol#L193-L199

Though no risk of reentrancy, it's a good practice to move all state changes to after the token is transferred.

Issue 7 (Non-critical) - Use all caps for constant

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/SupplySchedule.sol#L25

Issue 8 (Non-critical) - _pool parameter should be renamed _poolSize

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L880

_pool sounds like it's referring to the pool id.

Issue 9 (Non-critical) - Typo in comment

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L333

Issue 1 - Move require to top of function to avoid unnecessary state changes

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelMinter.sol#L368-L371

Issue 2 - assetDecimalsNormalizationValue can be made immutable

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L41

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/KnightingRound.sol#L63