Badger Citadel contest - berndartmueller's results

Lines of code

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

Vulnerability details

Contract StakedCitadelVester inherits from interface IVesting (in fact it does not as it is missing the necessary is IVesting statement, but it's assumed to inherit from IVesting) but wrongly implements the interface. The contract is expected to implement the function setupVesting() but it's wrongly called vest().

Impact

DoS withdrawing shares for tokens and therefore funds are stuck in contract and can not be withdrawn.

Proof of Concept

Following test will revert on xCitadel.withdraw(1e18); due to missing StakedCitadelVester.setupVesting() function:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.12;

import {BaseFixture} from "./BaseFixture.sol";
import {GlobalAccessControl} from "../GlobalAccessControl.sol";

contract StakedCitadelVesterTest is BaseFixture {
  function setUp() public override {
      BaseFixture.setUp();
  }

  function testWithdraw() public {
      vm.startPrank(governance);

      uint256 initialSupply = 10 * 1e18;

      citadel.mint(governance, initialSupply);
      citadel.approve(address(xCitadel), 1e18);
      xCitadel.deposit(1e18);

      assertEq(citadel.balanceOf(address(xCitadelVester)), 0);

      xCitadel.withdraw(1e18);

      (uint256 unlockBegin,,,) = xCitadelVester.vesting(address(governance));

      assertGt(unlockBegin, 0);
      assertGt(citadel.balanceOf(address(xCitadelVester)), 0);

      vm.stopPrank();
  }
}

Tools Used

Manual review

Recommended mitigation steps

Either rename StakedCitadelVester.vest() to StakedCitadelVester.setupVesting() or vice-versa.

Lines of code

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

Vulnerability details

Impact

The first depositor into StakedCitadel is able to maliciously manipulate the share price by depositing the lowest possible amount (1 wei) and then artificially blowing up the StakedCitadel Citadel token balance. Following depositors will loose their deposited funds due to precisions issues.

This is a well known issue, Uniswap and other protocols had similar issues when supply == 0.

Proof of Concept

1. Shark (attacker) deposits 1 wei via StakedCitadel.deposit()
2. Shark receives 1 wei StakedCitadel shares (due to shares = _amount on StakedCitadel.sol#L888)
3. Shark transfers 1 ether (1e18) tokens via citadel.transfer() to StakedCitadel to artificially blow up balance without minting new shares. StakedCitadel citadel token balance is now 1 ether + 1 wei -> share price is now very high (= 1000000000000000000001 wei ~ 1000 * 1e18)
4. Shrimp (victim) deposits 100 ether via StakedCitadel.deposit()
5. Shrimp receives 0 StakedCitadel shares

Shrimp receives 0 shares due to precision issue. Deposited funds are lost.

The formula shares = (_amount * totalSupply()) / _pool; see here calculates in case of such a high share price the following:

shares = (500000000000000000000 * 1) / 1000000000000000000001 => shares = 0

This is due to _pool > (_amount * totalSupply()).

Following test verifies this issue:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.12;

import {BaseFixture} from "./BaseFixture.sol";

contract StakedCitadelTest is BaseFixture {
  function setUp() public override {
      BaseFixture.setUp();
  }

  function testDeposit() public {
      vm.startPrank(governance);

      assertEq(xCitadel.balance(), 0);

      citadel.mint(shark, 1001 * 1e18);
      citadel.mint(shrimp, 1001 * 1e18);

      vm.stopPrank();

      vm.prank(address(shark));
      citadel.approve(address(xCitadel), type(uint256).max);

      vm.prank(address(shrimp));
      citadel.approve(address(xCitadel), type(uint256).max);

      vm.prank(address(shark));
      xCitadel.deposit(1);

      assertEq(xCitadel.balance(), 1);
      assertEq(xCitadel.balanceOf(address(shark)), 1);

      vm.prank(address(shark));
      citadel.transfer(address(xCitadel), 1000 * 1e18); // attacker transfers citadel token to vault

      assertEq(xCitadel.balance(), 1000 * 1e18 + 1);

      vm.prank(address(shrimp));
      xCitadel.deposit(500 * 1e18); // user deposits citadel token to vault

      assertGt(xCitadel.balanceOf(address(shrimp)), 0); // shrimp should receive shares but does _not_. Citadel tokens deposited are now lost.
  }
}

Fails as shrimp did receive 0 shares.

Tools Used

Manual review

Recommended mitigation steps

For the first deposit, mint fixed amount of shares, e.g. ONE_ETH

function _mintSharesFor(
    address recipient,
    uint256 _amount,
    uint256 _pool
) internal {
    uint256 shares;
    if (totalSupply() == 0) {
        shares = ONE_ETH; // @audit-info mint fixed amount of shares if supply is 0
    } else {
        shares = (_amount * totalSupply()) / _pool;
    }
    _mint(recipient, shares);
}

Following test verifies this fix:

function testCorrectDeposit() public {
    vm.startPrank(governance);

    assertEq(xCitadel.balance(), 0);

    citadel.mint(shark, 1001 * 1e18);
    citadel.mint(shrimp, 1001 * 1e18);

    vm.stopPrank();

    vm.prank(address(shark));
    citadel.approve(address(xCitadel), type(uint256).max);

    vm.prank(address(shrimp));
    citadel.approve(address(xCitadel), type(uint256).max);

    vm.prank(address(shark));
    xCitadel.deposit(1);

    assertEq(xCitadel.balance(), 1);
    assertEq(xCitadel.balanceOf(address(shark)), 1e18); // @audit-info shark received minimum amount of `1e18` shares

    vm.prank(address(shark));
    citadel.transfer(address(xCitadel), 1000 * 1e18); // attacker transfers citadel token to vault

    assertEq(xCitadel.balance(), 1000 * 1e18 + 1);

    vm.prank(address(shrimp));
    xCitadel.deposit(500 * 1e18); // user deposits citadel token to vault

    assertGt(xCitadel.balanceOf(address(shrimp)), 0); // @audit-info shrimp now receives shares > 0
}

Fix inspired by Uniswap V2 and Harvest Finance

Extra caution has to be put on initial deployment and on the first deposit into StakedCitadel. It's even recommended for the protocol owner to provide the initial deposit.

QA Report

It's recommended to have a very high test code coverage to spot critical issues. For this protocol many tests are missing for critical functions in the protocol (e.g. depositing Citadel into StakedCitadel to receive xCitadel).

Table of Contents

• Non-Critical Findings
  • NC-01: Use Solidity suffix days to specify units of time
  • NC-02: Contract implementations should inherit their interface
  • NC-03: Use token.decimals() of underlying token instead of constant ONE_ETH
  • NC-04: Use StakedCitadel.balance() consistently
• Low Risk
  • L-01: Discrepancies between the interface and implementation
  • L-02: transferFromDisabled storage variable is not implemented as comment implies
  • L-03: Zero-address checks are missing
  • L-04: Missing setter function for Funding.sol#citadelPriceInAssetOracle
  • L-05: Misleading comment for deposit() function of Funding.sol
  • L-06: Open @TODOs left in the code
  • L-07: Change of governance should use two-step process

Non-Critical Findings

NC-01: Use Solidity suffix days to specify units of time

Description

Suffixes like seconds, minutes, hours, days and weeks after literal numbers can be used to specify units of time where seconds are the base unit.

It's easier to read and helps to prevent issues.

Findings

StakedCitadel.sol#L34

Recommended mitigation steps

Use unit of time suffix:

Instead of

uint256 public constant INITIAL_VESTING_DURATION = 86400 * 21;

use

uint256 public constant INITIAL_VESTING_DURATION = 1 days * 21;

NC-02: Contract implementations should inherit their interface

Description

It's best practice for a contract to inherit from it's interface. This improves the contract's clarity and makes sure the contract implementation complies with the defined interface.

Findings

StakedCitadel.sol#L64
StakedCitadelVester.sol#L17
SupplySchedule.sol#L21
CitadelToken.sol#L8
GlobalAccessControl.sol#L19

Recommended mitigation steps

Inherit from the missing interface or contract.

NC-03: Use token.decimals() of underlying token instead of constant ONE_ETH

Description

Instead of defining the constant ONE_ETH with 18 decimals, use the actual token decimals of the underlying token.

Findings

StakedCitadel.sol#L68

Recommended mitigation steps

Use underlying token decimals. e.g.

Instead of

if (totalSupply() == 0) {
    return ONE_ETH;
}

use

if (totalSupply() == 0) {
    return 10**token.decimals(); // @audit-info use `token.decimals()` to make sure to really use same decimals as underlying token
}

NC-04: Use StakedCitadel.balance() consistently

Description

In StakedCitadel.sol the public view function StakedCitadel.balance() returns the total balance of the underlying token. As within the contract this balance is used multiple times across multiple functions, it's recommended to use this function to access the balance instead of using token.balanceOf(address(this)).

Findings

StakedCitadel.sol#L300
StakedCitadel.sol#L773
StakedCitadel.sol#L775
StakedCitadel.sol#L815
StakedCitadel.sol#L819

Recommended mitigation steps

Use function StakedCitadel.balance() instead of token.balanceOf(address(this)).

Low Risk

L-01: Discrepancies between the interface and implementation

Description

Contracts should implement the interface and adhere to it's specifications to prevent any issues using the contract.

Findings

GlobalAccessControl.sol#L19

Missing implementation for interface IGac defined functions enableTransferFrom and disableTransferFrom

Recommended mitigation steps

Either adapt interface to have same methods as contract implementation or adapt contract implementation to interface.

L-02: transferFromDisabled storage variable is not implemented as comment implies

Description

The comment implies that the storage variable transferFromDisabled is used and initialized in the constructor, but it's not.

// Should the function transferFrom be disabled
// NOTE: This is enforced at the contract level, the contract just allows the toggling of the bool
bool public transferFromDisabled; // Set to true in initialize

Findings

GlobalAccessControl.sol#L51

Recommended mitigation steps

Consider removing the unused variable or actually use it.

L-03: Zero-address checks are missing

Description

Zero-address checks are a best-practice for input validation of critical address parameters. While the codebase applies this to most most cases, there are many places where this is missing in constructors and setters.

Impact: Accidental use of zero-addresses may result in exceptions, burn fees/tokens or force redeployment of contracts.

Findings

lib/GlobalAccessControlManaged.sol#L32
lib/SettAccessControl.sol#L53
Funding.sol#L125
Funding.sol#L126
Funding.sol#L127
GlobalAccessControl.sol#L61

Recommended mitigation steps

Add zero-address checks.

L-04: Missing setter function for Funding.sol#citadelPriceInAssetOracle

Description

Currently there is no way to update the oracle citadelPriceInAssetOracle. To have the option to later update the storage variable, add a setter function to do so.

Findings

Funding.sol#L130

Recommended mitigation steps

Add setter function or change storage variable to immutable if it's not supposed to be updated later (for gas saving).

L-05: Misleading comment for deposit() function of Funding.sol

Description

Use of accurate comments helps read, audit and maintain code. Otherwise, it can be misleading and miss the flagging of or cause the introduction of vulnerabilities.

Findings

Funding.sol#L160

* @param _minCitadelOut ID of DAO to vote for // @audit-info wrong param description

Recommended mitigation steps

Update parameter documentation for _minCitadelOut

L-06: Open @TODOs left in the code

Description

There are several open TODOs left in the code.

Findings

SupplySchedule.sol#L159
Funding.sol#L15
Funding.sol#L61
Funding.sol#L183
KnightingRound.sol#L14
GlobalAccessControl.sol#L106

Recommended mitigation steps

Check, fix and remove the todos before it is deployed in production

L-07: Change of governance should use two-step process

Description

It's a best practice to use a two-step process for changes of critical settings like setGovernance().

Findings

lib/SettAccessControl.sol.sol#L51

Recommended mitigation steps

Consider implementing a two-step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

Gas Optimizations

Table of Contents

• G-01: Use unchecked {} primitive within for loops
• G-02: Public functions that could be declared external to save gas
• G-03: > 0 is less efficient than != 0 for unsigned integers
• G-04: Checking non-zero value can avoid an external call to save gas

G-01: Use unchecked {} primitive within for loops

Given the use of Solidity compiler >= 0.8.0, there are default arithmetic checks for mathematical operations which consume additional gas for such checks internally. In expressions where we are absolutely sure of no overflows/underflows, one can use the unchecked primitive to wrap such expressions to avoid checks and save gas.

Recommended mitigation steps

Adapt for loops to increase index variable within unchecked block. e.g

for (uint256 i = 0; i < length;) {
  // ...

  unchecked { ++i; }
}

Findings

SupplySchedule.sol#L208
CitadelMinter.sol#L152
CitadelMinter.sol#L344

G-02: Public functions that could be declared external to save gas

Description

Following functions should be declared external, as functions that are never called by the contract internally should be declared external to save gas.

Findings

StakedCitadel.sol#L284
SupplySchedule.sol#L79
lib/SettAccessControl.sol#L51
Funding.sol#L223

Recommended mitigation steps

Use the external attribute for functions never called from the contract.

G-03: > 0 is less efficient than != 0 for unsigned integers

Description

!= 0 is a cheaper (less gas) operation for unsigned integers within require statements compared to > 0.

Findings

StakedCitadel.sol#L835
StakedCitadel.sol#L908
StakedCitadelVester.sol#L138
SupplySchedule.sol#L61
SupplySchedule.sol#L91
SupplySchedule.sol#L180
lib/SafeERC20.sol#L96
Funding.sol#L170
Funding.sol#L209
Funding.sol#L322
Funding.sol#L340
Funding.sol#L424
Funding.sol#L452
CitadelMinter.sol#L270
CitadelMinter.sol#L343
KnightingRound.sol#L125
KnightingRound.sol#L129
KnightingRound.sol#L172
KnightingRound.sol#L184
KnightingRound.sol#L215
KnightingRound.sol#L313
KnightingRound.sol#L332
KnightingRound.sol#L411
interfaces/convex/BoringMath.sol#L20
interfaces/convex/BoringMath.sol#L102
interfaces/convex/BoringMath.sol#L122
interfaces/convex/BoringMath.sol#L142\

Recommended mitigation steps

Change > 0 to != 0.

G-04: Checking non-zero value can avoid an external call to save gas

Description

Checking if governanceRewardsFee > 0, strategistRewardsFee > 0 before making the safeTransfer call can save gas by avoiding this token transfer in such situations.

Findings

StakedCitadel.sol#L461
StakedCitadel.sol#L462\

Recommended mitigation steps

Add check for governanceRewardsFee > 0. E.g

if (governanceRewardsFee > 0) {
  IERC20Upgradeable(_token).safeTransfer(treasury, governanceRewardsFee);
}

Add check for strategistRewardsFee > 0. E.g

if (strategistRewardsFee > 0) {
  IERC20Upgradeable(_token).safeTransfer(
      strategist,
      strategistRewardsFee
  );
}