Badger Citadel contest - TrungOre's results

Lines of code

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

Vulnerability details

Impact

User can lose their fund because of incorrect function

Proof of Concept

In the comment, balance() function should return the total balance of citadel within sett and strategy system. But in the current code, the function just returns the balance within sett It can make users lose their token when strategy takes a part of fund for yield-generating activities (strategy can do it by using earn function). As you can see on the image above

• b = balance() (they are both equal to token.balanceOf(address(this)))
• shares <= totalSupply() (if not the transaction will be reverted because of _burn function) ==> r <= b

It will make the code never reach into if block and the fund won't ever be withdrawn out of strategy

Tools Used

Manual review

Recommended Mitigation Steps

Modify the function balance() in StakedCitadel.sol

function balance() public view returns (uint256) {
    return token.balanceOf(address(this))
            + token.balanceOf(strategy);
}

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/test/BaseFixture.sol#L92 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/test/BaseFixture.sol#L157 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadelVester.sol#L132

Vulnerability details

Impact

Users can't withdraw

Proof of Concept

As we can see in BaseFixture.sol, vesting contract of StakedCitadel is a StakedCitadelVester contract

• https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/test/BaseFixture.sol#L92
• https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/test/BaseFixture.sol#L157

In _withdraw function of StakedCitadel.sol, there is a call to function setupVesting of vesting contract, but there is no function which has a name like that in StakedCitadelVester.sol

Tools Used

Manual review

Recommended Mitigation Steps

use vest function instead of setupVesting https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadelVester.sol#L132

Lines of code

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

Vulnerability details

Impact

Users can get the wrong amount of citadel when depositing to funding

Proof of Concept

if funding.discount == 0, it won't go into if block https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L209 It make citadelAmount_ still equal to 0 in line 215 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L215

Tools Used

manual review

Recommended Mitigation Steps

modify getAmountOut function

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

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

        citadelAmount_ = citadelAmount_ / assetDecimalsNormalizationValue;
    }

Lines of code

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

Vulnerability details

Impact

User can't withdraw their token in their second vesting

Proof of Concept

it("user cannot claim their balance in second vest", async() => {
        // for easy to understand, i will set vesting duration = 10s, instead of 21 days
        await vester.setVestingDuration(10);

        // setup vesting for alice (1000 CTDL) at time 0
        await vester.setBlockTime(0);
        await vester.vest(
            alice.address, 
            1000, 
            0
        );

        // alice claim her reward at timestamp = 10
        await vester.setBlockTime(10);
        await vester.connect(alice).claim(
            alice.address, 
            1000
        );

        // alice get 1000 token reward 
        expect(await citadel.balanceOf(alice.address)).to.equal(1000);

        // setup another vesting for alice (1000 CTDL) at time 11
        await vester.setBlockTime(11);
        await vester.vest(
            alice.address, 
            1000, 
            11
        );

        // setup vesting for bob (1000 CTDL) at time 11
        await vester.vest(
            bob.address, 
            1000, 
            11
        );
        
        // bob and alice both claim their token at time 12, bob can claim 100 token when alice can claim nothing because of reverted transaction
        await vester.setBlockTime(12);
        expect(await vester.claimableBalance(bob.address)).to.equal(100);
        await expect(vester.claimableBalance(alice.address)).to.be.reverted;
    });

In example above, alice has 2 vesting with 1000 CTDL each, in the second vesting, she tries to claim her tokens after 1 second from unlockBegin. As normal (like bob) she might get 100 tokens, but unfortunately, she claims nothing, because the formula

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

give a negative result, which make tx revert.

• locked = 2000
• block.timestamp = 12
• vesting[recipient].unlockBegin = 11
• vesting[recipient].unlockBegin = 21
• claimed = 1000 ==> claimable = 200 - 1000 = -800 < 0

Tools Used

javascript

Recommended Mitigation Steps

Add a variable lastClaimTimestamp to store the last time that user has claimed the token. In another word, lastClaimTimestamp will be updated to block.timestamp for each time user claim.

And the function will be like this

function claimableBalance(address recipient) public view returns (uint256) {
        uint256 locked = vesting[recipient].lockedAmounts - vesting[recipient].claimedAmounts;
        // uint256 claimed = vesting[recipient].claimedAmounts;
        if (block.timestamp >= vesting[recipient].unlockEnd) {
            return locked;
        }
        return locked 
            * (min(block.timestamp, vesting[recipient].unlockEnd) - max(vesting[recipient].lastClaimTimestamp, vesting[recipient].unlockBegin))
            / (vesting[recipient].unlockEnd - max(vesting[recipient].lastClaimTimestamp, vesting[recipient].unlockBegin));
    }

[2022-04-badger-citadel] QA report

tags: c4, 2022-04-badger-citadel

safeApprove is deprecated

Afftected code

• https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelMinter.sol#L133
• https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelMinter.sol#L136
• https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/Funding.sol#L142

Proof of concept

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/5a75065659a65e65bb04890192e3a4bcb7917fff/contracts/token/ERC20/utils/SafeERC20.sol#L39

missing require minDiscount <= maxDiscount

Affected code

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

missing require minPrice <= maxPrice

Affected code

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

[2022-04-badger-citadel] Gas optimization

tags: c4, 2022-04-badger-citadel

totalTokenIn always < tokenInLimit

affected code

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

solution: place them into unchecked block

assetCumulativeFunded always < assetCap

afftected code

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

solution: place them into unchecked block

use delete instead of manual set value to 0

affected code

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

solution: use delete fundingPoolWeights[_pool] instead

use calldata instead of memory

affected code

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

redundant reentrancy

affected code

• https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelMinter.sol#L254
• https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelMinter.sol#L173
• https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/CitadelMinter.sol#L298

redundant fundingBps

affected code

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

Proof of concept

• Because fundingBps + stakingBps + lockingBps == MAX_BPS so we can deduce fundingBps from stakingBps and lockingBps