`StakedCitadelVester/claimableBalance()` has problems when users keep vesting.

[code423n4]

Lines of code

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

Vulnerability details

Impact

When users vest in StakedCitadelVester, the tokens will be locked in vest duration. Users can call claim() to get back their tokens. The claimable amount is calculated in claimableBalance(). Before the duration ends, the claimable amount is

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

It seems to be a fair claimable amount. However if users keep vesting more and more, the claimableBalance() will become unfair and sometimes revert.

Proof of Concept

• Alice vests 10000 CTDL, then vesting[recipient].lockedAmounts becomes 10000

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

    function vest(
        address recipient,
        uint256 _amount,
        uint256 _unlockBegin
    ) external {
        …
        vesting[recipient].lockedAmounts =
            vesting[recipient].lockedAmounts +
            _amount;
        vesting[recipient].unlockBegin = _unlockBegin;
        vesting[recipient].unlockEnd = _unlockBegin + vestingDuration;
        …
    }

• Alice claim 10000 CTDL after the duration ends, then vesting[msg.sender].claimedAmounts becomes 10000

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

    function claim(address recipient, uint256 amount) external {
        uint256 claimable = claimableBalance(msg.sender);
        if (amount > claimable) {
            amount = claimable;
        }
        if (amount != 0) {
            vesting[msg.sender].claimedAmounts =
                vesting[msg.sender].claimedAmounts +
                amount;
            vestingToken.safeTransfer(recipient, amount);
            emit Claimed(msg.sender, recipient, amount);
        }
    }

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

    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;
    }

• Alice then vest 100 CTDL, vesting[recipient].lockedAmounts becomes 10100

    function vest(
        address recipient,
        uint256 _amount,
        uint256 _unlockBegin
    ) external {
        …
        vesting[recipient].lockedAmounts =
            vesting[recipient].lockedAmounts +
            _amount;
        …
    }

• After half of vesting duration, Alice wants to claim some token. But it will revert in claimableBalance()

Since

locked = vesting[recipient].lockedAmounts = 10100
claimed = vesting[recipient].claimedAmounts = 10000

 ((locked * (block.timestamp - vesting[recipient].unlockBegin)) /
                (vesting[recipient].unlockEnd -
                    vesting[recipient].unlockBegin))   =   10100 * (1/2) = 5050

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

This line will revert

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

• Alice can still claim all the tokens after the vesting duration ends, but as Alice keeps vesting, it gets harder to claim tokens back during vesting duration. Since lockedAmounts will keep growing.

Tools Used

vim

Recommended Mitigation Steps

vest() could be modified like the following code.

    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].claimedAmounts ;
        vesting[recipient].claimedAmounts = 0;
        vesting[recipient].unlockBegin = _unlockBegin;
        vesting[recipient].unlockEnd = _unlockBegin + vestingDuration;

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

[GalloDaSballo]

@dapp-whisperer Wdyt?

[shuklaayush]

158