code-423n4 / 2024-01-curves-findings

0 stars 0 forks source link

Curves::_buyCurvesToken(), Excess of Eth received is not refunded back to the user. #48

Open c4-bot-3 opened 8 months ago

c4-bot-3 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L211-L216 https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L263-L280

Vulnerability details

Impact

In the buyCurvesToken(), the logic check for the msg.value to be greater than price + fees. This is fine, since if the price moves after the estimates was provided to the user, the above validation checks if the funds received are sufficient to proceed with the transaction.

But, at the same time, it is equally important to refund any excess eth received which is not being done.

Proof of Concept

Refer to the below code where the validation ensures that msg.value is not less than price + total fee.

   if (msg.value < price + totalFee) revert InsufficientPayment();

But, incase any additional funds were received, the same should be returned back to the caller.

Tools Used

Manual review.

Recommended Mitigation Steps

uint256 excess = msg.value - (price + totalFee); if excess > 0, refund the amount back to the caller.

Assessed type

ETH-Transfer

c4-pre-sort commented 7 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 7 months ago

raymondfam marked the issue as primary issue

andresaiello commented 7 months ago

valid but not critical. friend tech does not refound in fact

c4-sponsor commented 7 months ago

andresaiello marked the issue as disagree with severity

c4-sponsor commented 7 months ago

andresaiello (sponsor) confirmed

c4-judge commented 7 months ago

alcueca changed the severity to 2 (Med Risk)

c4-judge commented 7 months ago

alcueca marked the issue as satisfactory

c4-judge commented 6 months ago

alcueca marked the issue as selected for report