Badger Citadel contest - gzeon's results

Lines of code

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

Vulnerability details

Impact

When vest is called by xCTDL vault, the previous amount will re-lock according to the new vesting timeline. While this is as described in L127, claimableBalance might revert due to underflow if vesting[recipient].claimedAmounts > 0 because the user will need to vest the claimedAmounts again which should not be an expected behavior as it is already vested.

Proof of Concept

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

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

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

        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;

Recommended Mitigation Steps

Reset claimedAmounts on new vest

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

Low Risk

tokenOutPrice can be decreased but earlier buyer still pay the higher price

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

Non-Critical

Allow buy maximum amount when exceeding total amount in KnightingRound

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

Unresolved TODOs

./src/SupplySchedule.sol:159:        // TODO: Require this epoch is in the future. What happens if no data is set? (It just fails to mint until set)
./src/Funding.sol:15: * TODO: Better revert strings
./src/Funding.sol:61:    // TODO: we should conform to some interface here
./src/Funding.sol:183:        // TODO: Check gas costs. How does this relate to market buying if you do want to deposit to xCTDL?
./src/KnightingRound.sol:14: * TODO: Better revert strings
./src/GlobalAccessControl.sol:106:    /// TODO: Add string -> hash EnumerableSet to a new RoleRegistry contract for easy on-chain viewing.

> 0 is less efficient than != 0 for uint in require condition

Ref: https://twitter.com/GalloDaSballo/status/1485430908165443590

src/StakedCitadelVester.sol:138:        require(_amount > 0, "StakedCitadelVester: cannot vest 0");
src/Funding.sol:170:        require(_assetAmountIn > 0, "_assetAmountIn must not be 0");
src/Funding.sol:322:        require(amount > 0, "nothing to sweep");
src/Funding.sol:340:        require(amount > 0, "nothing to claim");
src/Funding.sol:424:        require(_citadelPriceInAsset > 0, "citadel price must not be zero");
src/Funding.sol:452:        require(_citadelPriceInAsset > 0, "citadel price must not be zero");
src/CitadelMinter.sol:343:        require(length > 0, "CitadelMinter: no funding pools");
src/KnightingRound.sol:172:        require(_tokenInAmount > 0, "_tokenInAmount should be > 0");
src/KnightingRound.sol:215:        require(tokenOutAmount_ > 0, "nothing to claim");
src/KnightingRound.sol:411:        require(amount > 0, "nothing to sweep");

Float multiplication optimization

We can use the following function to save gas on float multiplications

// out = x * y unchecked{/} z
function fmul(uint256 x, uint256 y, uint256 z) internal pure returns(uint256 out){
assembly{
if iszero(eq(div(mul(x,y),x),y)) {revert(0,0)}
out := div(mul(x,y),z)
}
}

e.g.

src/StakedCitadel.sol:300:        return (token.balanceOf(address(this)) * toEarnBps) / MAX_BPS;
src/StakedCitadel.sol:852:        uint256 fee = (amount * feeBps) / MAX_BPS;
src/test/MintAndDistribute.t.sol:88:        uint expectedToFunding = expectedMint * fundingBps / MAX_BPS;
src/test/MintAndDistribute.t.sol:100:        uint expectedToStakers = expectedMint * stakingBps / MAX_BPS;
src/test/MintAndDistribute.t.sol:111:        uint expectedToLockers = expectedMint * stakingBps / MAX_BPS;
src/CitadelMinter.sol:191:            lockingAmount = (mintable * cachedLockingBps) / MAX_BPS;
src/CitadelMinter.sol:215:            stakingAmount = (mintable * cachedStakingBps) / MAX_BPS;
src/CitadelMinter.sol:228:            fundingAmount = (mintable * cachedFundingBps) / MAX_BPS;

Cache vesting parameters

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

    function claimableBalance(address recipient) public view returns (uint256) {
        uint256 locked = vesting[recipient].lockedAmounts;
        uint256 claimed = vesting[recipient].claimedAmounts;
        uint256 begintime = vesting[recipient].unlockBegin;
        uint256 endtime = vesting[recipient].unlockEnd;

        if (block.timestamp >= endtime) {
            return locked - claimed;
        }
        return
            ((locked * (block.timestamp - begintime)) /
                (endtime - begintime)) - claimed;
    }

Unchecked safe math

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

        require(
            funding.assetCumulativeFunded + _assetAmountIn <= funding.assetCap,
            "asset funding cap exceeded"
        );
        unchecked{
            funding.assetCumulativeFunded = funding.assetCumulativeFunded + _assetAmountIn;
        }

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

        unchecked{
                totalTokenIn = totalTokenIn + _tokenInAmount;
        }

Use custom errors

Solidity ^0.8.4 allow the use of custom errors to optimize gas usage. https://blog.soliditylang.org/2021/04/21/custom-errors/