`cvxPerVotium()` calculation will return zero if all CVX tokens are pending withdrawal as obligations

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L144-L149

Vulnerability details

Summary

The implementation of cvxPerVotium() contains an edge case that causes it to return an invalid zero value price.

Impact

The cvxPerVotium() function present in the VotingStrategy contract is used to measure the number of held CVX tokens per vAfEth.

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L144-L149

144:     function cvxPerVotium() public view returns (uint256) {
145:         uint256 supply = totalSupply();
146:         uint256 totalCvx = cvxInSystem();
147:         if (supply == 0 || totalCvx == 0) return 1e18;
148:         return ((totalCvx - cvxUnlockObligations) * 1e18) / supply;
149:     }

The implementation basically divides the number of deposited CVX tokens over the total supply of vAfEth, while carefully subtracting any balance that is pending to be withdrawn (cvxUnlockObligations).

However, if all CVX tokens are currently pending withdrawal, then cvxUnlockObligations == totalCvx, causing cvxPerVotium() to return zero. This case should be similar to totalCvx == 0 and should return 1e18 instead of zero.

The cvxPerVotium() function is used in several places throughout the codebase:

• In VotingStrategy::deposit(), to calculate the number of minted tokens.
• In VotingStrategy::requestWithdraw(), to calculate the amount of owed CVX tokens.
• In VotingStrategy::price(), to calculate the price in ETH of the vAfEth token.
  • The VotiumStrategy price is then used in other functions, carrying the error forward:
  • In AfEth::price(), to calculate the price in ETH of the AfEth token.
  • In AfEth::deposit(), to calculate the number of minted tokens of new deposits.
• In AfEth::depositRewards(), to calculate the Votium strategy TVL.

This means that under this scenario, all these other critical functions will be affected.

Recommendation

The implementation of cvxPerVotium() can be fixed by subtracting the CVX obligations before doing the totalCvx == 0 comparison. The edge case is then removed, since now totalCvx should be zero if all CVX is pending to be withdrawn.

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

Assessed type

Other

[elmutt]

https://github.com/asymmetryfinance/afeth/pull/166

[c4-judge]

0xleastwood marked the issue as primary issue

[0xleastwood]

This makes sense to implement.

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-sponsor]

elmutt (sponsor) confirmed

[d3e4]

The only time cvxUnlockObligations is increased is in VotiumStrategy.requestWithdraw(), but in the same function the tokens are also burned. This means that when the last supply is withdrawn (making cvxUnlockObligations == totalCvx) the last supply is also burned, which makes totalSupply() == 0 so that the check if (supply == 0 || totalCvx == 0) return 1e18; is entered after all. Thus the described scenario cannot happen.

[0xleastwood]

Agreed, totalSupply() would return zero and return 1e18 so this is not an issue.

[c4-judge]

0xleastwood marked the issue as unsatisfactory: Invalid