Badger Citadel contest - cccz's results

Lines of code

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

Vulnerability details

Impact

In the StakedCitadel contract, the balance() function only returns the token balance in the StakedCitadel contract, without adding the token balance in the strategy. This results in a smaller return value of balance() in other functions such as _depositFor, _withdraw functions, thus minting more shares or withdrawing less tokens for the user.

    /// @notice Gives the total balance of the underlying token within the sett and strategy system.
    /// @return Balance of token handled by the sett.
    function balance() public view returns (uint256) {
        return token.balanceOf(address(this));
    }

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Correctly implement the balance() function

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

Lines of code

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

Vulnerability details

Impact

In the _withdraw function of the StakedCitadel contract, the setupVesting function of vesting is called, while in the StakedCitadelVester contract, the function name is vest, which will cause the _withdraw function to fail, so that the user cannot withdraw the tokens.

        IVesting(vesting).setupVesting(msg.sender, _amount, block.timestamp);
        token.safeTransfer(vesting, _amount);
        ...
    function vest(
        address recipient,
        uint256 _amount,
        uint256 _unlockBegin
    ) external {
        require(msg.sender == vault, "StakedCitadelVester: only xCTDL vault");
        require(_amount > 0, "StakedCitadelVester: cannot vest 0");

        vesting[recipient].lockedAmounts =
            vesting[recipient].lockedAmounts +
            _amount;
        vesting[recipient].unlockBegin = _unlockBegin;
        vesting[recipient].unlockEnd = _unlockBegin + vestingDuration;

        emit Vest(
            recipient,
            vesting[recipient].lockedAmounts,
            _unlockBegin,
            vesting[recipient].unlockEnd
        );
    }

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Use the correct function name

interface IVesting {
    function vest(
        address recipient,
        uint256 _amount,
        uint256 _unlockBegin
    ) external;
}
...
IVesting(vesting).vest(msg.sender, _amount, block.timestamp);
token.safeTransfer(vesting, _amount);

Lines of code

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

Vulnerability details

Impact

In the buy function of the KnightingRound contract, the number of tokens obtained by the user is determined by the tokenOutPrice parameter, and tokenOutPrice can set in the setTokenOutPrice function. But if the setTokenOutPrice and buy functions are executed in the same block, the user may suffer losses because of the new tokenOutPrice. Consider the following scenarios: The current tokenOutPrice is 1000, the user thinks the price is appropriate, and the buy function is called. But at this time, the setTokenOutPrice function is called, setting the tokenOutPrice to 500, and this transaction occurs before executing the buy function, causing the user to execute the buy function in the case of tokenOutPrice 500.

Proof of Concept

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L193 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L234-L242 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/KnightingRound.sol#L327-L339

Tools Used

None

Recommended Mitigation Steps

Consider adding tokenOutMin parameters in the buy function and adding code in the buy function:

require(tokenOutAmount_ >= tokenOutMin)

Lines of code

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

Vulnerability details

Impact

The claimableBalance function of the StakedCitadelVester contract is used to calculate the number of tokens vested linearly by the user. But there is an error in the calculation here. Consider the following scenario: User locked 10000 tokens and vested all tokens after 21 days. At this point lockedAmounts = 10000, claimedAmounts = 10000. The user then locks another 100 tokens and wants to vest part of the tokens linearly after 20 days. According to the claimableBalance function

claimable = (10000+100) * 20 days / 21 days - 10000 = 9619 - 10000.

An overflow occurred and the transaction execution failed.

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

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Modify the claimableBalance function

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