Percentage calculation could leave unused ETH leftovers in AfEth deposit

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L160

Vulnerability details

Summary

The logic used to split the deposit amount between SafEth and the Votium strategy can leave some leftovers that will be ignored by the implementation.

Impact

Deposits in the AfEth contract are split based on a configured ratio. One portion of the split goes to SafEth, while the other is deposited in the Votium strategy.

To calculate the split, the implementation multiplies the deposit amount by ratio for the SafEth portion, and the deposit amount by 1e18 - ratio for the VotingStrategy portion.

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L148-L169

148:     function deposit(uint256 _minout) external payable virtual {
149:         if (pauseDeposit) revert Paused();
150:         uint256 amount = msg.value;
151:         uint256 priceBeforeDeposit = price();
152:         uint256 totalValue;
153: 
154:         AbstractStrategy vStrategy = AbstractStrategy(vEthAddress);
155: 
156:         uint256 sValue = (amount * ratio) / 1e18;
157:         uint256 sMinted = sValue > 0
158:             ? ISafEth(SAF_ETH_ADDRESS).stake{value: sValue}(0)
159:             : 0;
160:         uint256 vValue = (amount * (1e18 - ratio)) / 1e18;
161:         uint256 vMinted = vValue > 0 ? vStrategy.deposit{value: vValue}() : 0;
162:         totalValue +=
163:             (sMinted * ISafEth(SAF_ETH_ADDRESS).approxPrice(true)) +
164:             (vMinted * vStrategy.price());
165:         if (totalValue == 0) revert FailedToDeposit();
166:         uint256 amountToMint = totalValue / priceBeforeDeposit;
167:         if (amountToMint < _minout) revert BelowMinOut();
168:         _mint(msg.sender, amountToMint);
169:     }

Because of rounding and limited precision integer math, the split could potentially not cover the entirety of the deposit, i.e. sValue + vValue != amount. There could be some leftovers from both calculations that won't be considered in the deposit.

Recommendation

Instead of calculating vValue using 1e18 - ratio, simply use difference between the deposited amount and the calculated SafEth amount (sValue).

uint256 vValue = amount - sValue;

Assessed type

Other

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-sponsor]

elmutt (sponsor) confirmed

[Rassska]

@0xleastwood, Just wanna understand the impact behind this issue. Is it possible to run into the case, where the leftover will be more than a small delta == 1wei? Even if we somehow make the calculations over the prime numbers, it might not be feasible, but prob I'm missing something here.

The invariant is not broken(e.g user funds are 100% allocated between strategies)

[0xleastwood]

Yes it would be. We have two calculations sValue = (amount * ratio) / 1e18 and vValue = (amount * (1e18 - ratio)) / 1e18 and sValue + vValue will most likely not equal the full amount passed as msg.value. Not sure to what extent there would be rounding but it would be at least 1 wei. If we know what the cap might be, I'm not opposed to downgrading this.

[0xleastwood]

Okay tested this out a bit, it appears rounding is capped at 1 wei.

[0xleastwood]

Downgrading to QA because this is super minor.

[c4-judge]

0xleastwood marked the issue as not selected for report

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[0xleastwood]

Thanks for flagging this.