code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

The typical usage of max/min amounts in transitions is that it enforces a threshold limit on the amount transferred while allowing the max/min amounts to be transacted. However the implementation in repay functions requires that the amount <= maxAmount (and reverting if not) instead of repaying maxAmount if amount > maxAmount.

Unaware users may fail their repayments and lose gas because of this atypical interpretation of maxAmount.

This was likely implemented to address this front-running finding from the previous contest: https://github.com/code-423n4/2021-07-wildcredit-findings/issues/125 but is likely a much harsher approach to revert than pay up to the maxAmount.

Proof of Concept

https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/LendingPair.sol#L212-L219

https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/LendingPair.sol#L221-L235

Tools Used

Manual Analysis

Recommended Mitigation Steps

Implement the typical interpretation of maxAmount and if not, highlight this explicitly in the user documentation.

talegift commented 3 years ago

Revert on invalid user input (maxAmount being lower than needed) is not a bug in the code.

I'm also not sure if we can consider suggested replacement behavior as "typical". The most famous example, Uniswap, also reverts when the output swap is lower than what was set as the limit.

Using the same logic, a swap function in Uniswap might calculate your price from the input and minOutput, and instead of reverting, perform a partial swap based on your input parameters.

Since the function is named repayAll, unless the execution really repays all debt, I think it's more suitable to revert it. If the function was named repayUpTo, suggested behavior would make more sense.

ghoul-sol commented 3 years ago

per sponsor comment, invalid

code-423n4 / 2021-09-wildcredit-findings