Sole depositor in the VotiumStrategy contract can inflate cvxPerVotium() to steal subsequent deposits

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L39-L46

Vulnerability details

Bug Description

In VotiumStrategyCore.sol, the cvxInSystem() function returns the total amount of CVX in the protocol:

VotiumStrategyCore.sol#L133-L138

    function cvxInSystem() public view returns (uint256) {
        (uint256 total, , , ) = ILockedCvx(VLCVX_ADDRESS).lockedBalances(
            address(this)
        );
        return total + IERC20(CVX_ADDRESS).balanceOf(address(this));
    }

The total amount of CVX is sum of the amount of locked CVX and the contract's current CVX balance. An attacker can "donate" CVX to the protocol to cause cvxInSystem() to increase by doing either of the following:

1. Directly transfer CVX to the VotiumStrategy contract.
2. Call depositRewards() with ETH, which will exchange ETH for CVX and hold it in the contract.

cvxInSystem() is called by cvxPerVotium() to determine the amount of CVX per vAfEth:

VotiumStrategyCore.sol#L144-L149

    function cvxPerVotium() public view returns (uint256) {
        uint256 supply = totalSupply();
        uint256 totalCvx = cvxInSystem();
        if (supply == 0 || totalCvx == 0) return 1e18;
        return ((totalCvx - cvxUnlockObligations) * 1e18) / supply;
    }

Where:

• cvxUnlockObligations is the amount of CVX for pending withdrawals.

As seen from above, cvxPerVotium() is essentially calculated from cvxInSystem() divided by the total supply of vAfEth. Therefore, if cvxInSystem() increases, cvxPerVotium() will also increase proportionally.

This becomes problematic as cvxPerVotium() is used to determine the amount of vAfEth to mint to the depositor when deposit() is called:

VotiumStrategy.sol#L39-L46

    function deposit() public payable override returns (uint256 mintAmount) {
        uint256 priceBefore = cvxPerVotium();
        uint256 cvxAmount = buyCvx(msg.value);
        IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmount);
        ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmount, 0);
        mintAmount = ((cvxAmount * 1e18) / priceBefore);
        _mint(msg.sender, mintAmount);
    }

The amount of vAfEth to mint is determined by the caller's deposit (cvxAmount) is divided by cvxPerVotium().

However, when the total supply of vAfEth is extremely small, an attacker can artificially inflate cxvPerVotium() by "donating" a huge amount of CVX to the protocol. If priceBefore becomes sufficiently large, cvxAmount * 1e18 / priceBefore will round down to 0, causing subsequent depositors to receive 0 vAfEth.

Impact

When the total supply of the VotiumStrategy contract is extremely small, an attacker can force mintAmount to round down to 0 in deposit(), thereby minting 0 vAfEth to subsequent depositors. This will cause all future deposits to accrue to the attacker's vAfEth, leading to a theft of funds from future depositors.

Proof of Concept

Assume that the VotiumStrategy contract is newly deployed and has no depositors yet.

Alice calls deposit() with an extremely small msg.value:

• buyCvx() returns cvxAmount = 1.
• Since this is the first deposit, cvxPerVotium() is 1e18.
• 1 vAfEth is minted to Alice as:

mintAmount = ((cvxAmount * 1e18) / priceBefore) = ((1 * 1e18) / 1e18) = 1

Alice directly transfers 1000 CVX to the contract. Now, if cvxPerVotium() is called:

• totalSupply() = 1
• cvxInSystem() = 1000e18
• cvxUnlockObligations = 0
• Therefore, it returns 1e36 as:

((totalCvx - cvxUnlockObligations) * 1e18) / supply = ((1000e18 - 0) * 1e18) / 1 = 1e39

Bob calls deposit() with ETH worth 500 CVX:

• buyCvx() returns cvxAmount = 500e18
• As shown above, cvxPerVotium() returns 1e39.
• mintAmount will round down to 0 as:

mintAmount = ((cvxAmount * 1e18) / priceBefore) = ((500e18 * 1e18) / 1e39) = 0

• Therefore, Bob does not receive any vAfEth for his deposit.

In this scenario, mintAmount will always round down to 0 if Bob deposits less than 1000 CVX worth of ETH. All of the CVX deposited by Bob accrues to Alice's 1 vAfEth, resulting in a loss of funds for Bob.

Note that another possible way for totalSupply() to become 1 is if everyone withdraws and Alice happens to be the last depositor in the VotiumStrategy contract. She can then withdraw all her vAfEth except 1 by calling requestWithdraw() and withdraw().

Recommended Mitigation

In deposit(), consider minting x amount of vAfEth to a dead address when totalSupply() == 0:

    function deposit() public payable override returns (uint256 mintAmount) {
+       if (totalSupply() == 0) {
+           _mint(address(0), 1000);
+       }
        
        uint256 priceBefore = cvxPerVotium();
        uint256 cvxAmount = buyCvx(msg.value);
        IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmount);
        ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmount, 0);
        mintAmount = ((cvxAmount * 1e18) / priceBefore);
        _mint(msg.sender, mintAmount);
    }

This ensures that totalSupply() will never be low enough for an attacker to inflate cvxPerVotium() significantly.

Assessed type

DoS

[elmutt]

nice examples of the problem. @toshiSat I think the easiest solution is to just do a seed deposit so we dont have to worry about or analyze any math/contract changes

[toshiSat]

Yes, the plan is to do a seed deposit with the multisig once launched

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as duplicate of #35

[c4-judge]

0xleastwood marked the issue as satisfactory

[c4-judge]

0xleastwood changed the severity to 2 (Med Risk)

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood

[c4-judge]

0xleastwood marked the issue as not a duplicate

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood

[c4-judge]

0xleastwood marked the issue as duplicate of #35