Badger Citadel contest - reassor's results

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadelVester.sol#L132-L136 https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/interfaces/citadel/IVesting.sol#L5-L9 https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/StakedCitadel.sol#L830

Vulnerability details

Impact

Contract StakedCitadel uses _withdraw internal function for withdrawing tokens which then are supposed to be sent to StakedCitadelVester contract. Interface IVesting is used to interact with StakedCitadelVester contract. The issue is that there is function name mismatch between interface and its implementation which ends up with revert of every transaction that tries to withdraw staked tokens. The staked tokens are effectively lost.

1. Contract StakedCitadelVester implements vest function:

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

2. In IVesting is defined setupVesting function:

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

3. Then StakedCitadel in _withdraw function tries to send it to StakedCitadelVester contract:

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

4. Every transaction that tries to withdraw tokens withdraw, withdrawAll ends up with revert.

Proof of Concept

• https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadelVester.sol#L132-L136
• https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/interfaces/citadel/IVesting.sol#L5-L9
• https://github.com/code-423n4/2022-04-badger-citadel/blob/dab143a990a9c355578fbb15cd3c884614e33f42/src/StakedCitadel.sol#L830

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

The solution is to properly implement interface in StakedCitadelVester with setupVesting function:

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

Alternative solution is changing IVesting interface the function setupVesting to vest:

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

And invoking vest in StakedCitadel contract:

IVesting(vesting).vest(msg.sender, _amount, block.timestamp)

Lines of code

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

Vulnerability details

Impact

Function.buy buys the tokens for whatever price is set as tokenOutPrice. This might lead to accidental collisions or front-running attacks when user is trying to buy the tokens and his transaction is being included after the transaction of changing the price of the token via setTokenOutPrice.

Scenario:

1. User wants to buy tokens and can see price tokenOutPrice
2. User likes the price and issues a transaction to buy tokens
3. At the same time CONTRACT_GOVERNANCE_ROLE account is increasing tokenOutPrice through setTokenOutPrice
4. setTokenOutPrice transaction is included before user's buy transaction
5. User buys tokens with the price he was not aware of

Another variation of this attack can be performed using front-running.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add additional parameter uint256 believedPrice to KnightingRound.buy function and check if believedPrice is equal to tokenOutPrice.

1. Missing zero address checks

Risk

Low

Impact

Project's contracts are missing zero address checks for setting state variables and using addresses for internal transaction logic. Setting some of the state variables to the zero address, whether intentional or not, can break the protocol functionality. Adding these checks consistently would prevent this scenario.

Proof of Concept

CitadelToken.sol

• address _gac - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelToken.sol#L25
• address dest - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelToken.sol#L40

GlobalAccessControl.sol

• address _initialContractGovernance - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/GlobalAccessControl.sol#L61

Funding.sol

• address _gac - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L105
• address _citadel - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L106
• address _asset - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L107
• address _xCitadel - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L108
• address _discountManager - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L373
• address _token - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L315

KnightingRound.sol

• address _globalAccessControl - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/KnightingRound.sol#L110
• address _tokenOut - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/KnightingRound.sol#L111
• address _tokenIn - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/KnightingRound.sol#L112

StakedCitadelVester.sol

• address _gac - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadelVester.sol#L60
• address recipient - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadelVester.sol#L85

SupplySchedule.sol

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

GlobalAccessControlManaged.sol

• address _globalAccessControl - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/lib/GlobalAccessControlManaged.sol#L27

SettAccessControl.sol

• address _strategist - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/lib/SettAccessControl.sol#L37
• address _keeper - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/lib/SettAccessControl.sol#L44
• address _governance - https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/lib/SettAccessControl.sol#L51

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add zero address checks to listed functions.

2. Funding.setDiscountLimits missing validation

Risk

Low

Impact

Function Funding.setDiscountLimits does not properly validate arguments. There is a missing check in case _minDiscount is bigger than _maxDiscount. Setting funding.maxDiscount to smaller value than funding.minDiscount might lead to undefined behavior.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add check if passed _minDiscount is smaller or equal to _maxDiscount.

3. Funding.claimAssetToTreasury missing nonreentrant modifier

Risk

Low

Impact

Most of Funding contract functions contain nonReentrant modifier that protects against reentrancy vulnerabilities. Function claimAssetToTreasury is missing that check.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add nonReentrant modifier to function Funding.claimAssetToTreasury.

4. Division by zero

Risk

Non-Critical

Impact

Function _withdraw of StakedCitadel contract performs calculations to retrieve tokens based on the shares. One of the calculations lead to division by zero if the amount of shares totalSupply is equal to 0.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to check if totalSupply is not equal to 0.

5. Sweep asset token bypass multiple addresses

Risk

Non-Critical

Impact

Contract Funding and KnightingRound use ERC20 tokens as an asset token and implement sweep mechanism to withdraw any tokens that have been accidentally sent to the contract(s). Function sweep checks if the the address of the token is the one used by the contract but this might be bypassed if the ERC20 token has multiple addresses.

Reference:

• https://github.com/d-xo/weird-erc20#multiple-token-addresses

Tools Used

Manual Review / VSCode

Proof of Concept

• https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L323-L326
• https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/KnightingRound.sol#L405

Recommended Mitigation Steps

It is recommended to validate asset tokens to make sure that used ERC20 token has not multiple addresses.

6. Usage of booleans in expressions

Risk

Non-Critical

Impact

Modifier onlyWhenPriceNotFlagged of Funding contract uses false boolean expression for comparing.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to use !citadelPriceFlag expression in require statement.

7. Typos in error messages

Risk

Non-Critical

Impact

Multiple functions contains typos in their error messages.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to fix typos.