Open sherlock-admin opened 1 year ago
Repayment can only make the position less risky, so don't see a need for frontrunning/stale TX protection. Documentation should clearly state this behavior.
I do see the issue being valid. As sponsor said, they don't see a need to implement a safeguard, but the watson demonstrated that users can suffer a loss if non-favorable repays are executed. Could keep the medium.
re discussed within team and we're going to provide a change for this behavior: https://github.com/ajna-finance/contracts/pull/914
Will keep the medium severity
re discussed within team and we're going to provide a change for this behavior: ajna-finance/contracts#914
Looks ok, limitIndex_
in repayDebt()
is now effective in all the cases.
But it looks like when vars.repay == vars.pull == false
the check will always revert the call as newPrice_ = result_.newLup == 0
:
function _revertIfPriceDroppedBelowLimit(
uint256 newPrice_,
uint256 limitIndex_
) pure {
if (newPrice_ < _priceAt(limitIndex_)) revert LimitIndexExceeded();
}
Consider:
// check limit price and revert if price dropped below
- _revertIfPriceDroppedBelowLimit(result_.newLup, limitIndex_);
+ if (vars.pull || vars.repay) _revertIfPriceDroppedBelowLimit(result_.newLup, limitIndex_);
re discussed within team and we're going to provide a change for this behavior: ajna-finance/contracts#914
Looks ok,
limitIndex_
inrepayDebt()
is now effective in all the cases.But it looks like when
vars.repay == vars.pull == false
the check will always revert the call asnewPrice_ = result_.newLup == 0
:function _revertIfPriceDroppedBelowLimit( uint256 newPrice_, uint256 limitIndex_ ) pure { if (newPrice_ < _priceAt(limitIndex_)) revert LimitIndexExceeded(); }
Consider:
// check limit price and revert if price dropped below - _revertIfPriceDroppedBelowLimit(result_.newLup, limitIndex_); + if (vars.pull || vars.repay) _revertIfPriceDroppedBelowLimit(result_.newLup, limitIndex_);
in that case we early revert with InvalidAmount at L288 https://github.com/ajna-finance/contracts/blob/0332f341856e1efe4da8bb675886c8cfbee57b71/src/libraries/external/BorrowerActions.sol#L288
// revert if no amount to pull or repay
if (!vars.repay && !vars.pull) revert InvalidAmount();
so later check not needed
Looks ok
hyh
medium
Limit index isn't checked in repayDebt, so user control is void
Summary
repayDebt() resulting LUP
_revertIfPriceDroppedBelowLimit()
check is not performed in the case of pure debt repayment without collateral pulling.Vulnerability Detail
LUP will move (up or no change) as a result of debt repayment and repayDebt() have
limitIndex_
argument. As a part of multi-position strategy a user might not be satisfied with repay results if LUP has increased not substantially enough.I.e. there is a user control argument, it is detrimental from UX perspective to request, but not use it, as for any reason a borrower might want to control for that move: they might expect the final level to be somewhere, as an example for the sake of other loans of that borrower.
Impact
Unfavorable repayDebt() operations will be executed and the borrowers, whose strategies were dependent on the realized LUP move, can suffer a loss.
Probability of execution is high (no prerequisites, current ordinary behavior), while the probability of the following loss is medium, so placing the severity to be medium.
Code Snippet
There is a
limitIndex_
parameter in repayDebt():https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/ERC20Pool.sol#L208-L232
Currently
_revertIfPriceDroppedBelowLimit()
is done on collateral pulling only:https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/libraries/external/BorrowerActions.sol#L365-L375
Tool used
Manual Review
Recommendation
Consider adding the same check in the repayment part:
https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/libraries/external/BorrowerActions.sol#L328
If no repay or pull it looks ok to skip the check to save gas.