Open sherlock-admin4 opened 1 month ago
I think this issue only happens when the underlying market protocol we integrate doesn't implement the withdraw()
function correctly, which it succeeded and returned true but the withdrawal amount was less than what the caller requested.
But to tight things up on our end, I think we can add another check on the actual withdrawal amount.
The protocol team fixed this issue in the following PRs/commits: https://github.com/unioncredit/union-v2-contracts/pull/175
The protocol team fixed this issue in the following PRs/commits: unioncredit/union-v2-contracts#175
Fix looks ok
hyh
Medium
Minimum borrow amount can be surpassed and borrower can be treated as being overdue earlier than their actual overdue time
Summary
It is possible to borrow less than
_minBorrow
and preliminary be marked as overdue whenassetManager
have temporary fund access limitations.Vulnerability Detail
UToken's
borrow()
can be effectively run with lesser amount than_minBorrow
when it is a liquidity shortage in the asset manager's underlying markets and they can return only some dust amount or nothing at all. In these casesborrow()
call will still be concluded. Particularly, it is possible to run it with zero amount whenassetManager
cannot access liquidity.In that case the borrower, if they borrow for the first time after full repay, will not have their
lastRepay
field reset on a subsequent material borrow operations as it will already be set on zero amount borrow before. As a result such borrowers can be effectively overdue for the system way before the actual overdue time passes for them.Impact
_minBorrow
threshold can be violated when market conditions restrictassetManager
withdrawals. A user can havelastRepay
set earlier than time of obtaining the funds, which will mark them overdue before the actual overdue time comes by. This will have a material adverse impact both on such a borrower (for themcheckIsOverdue
will be true, so they won't be able to borrow or create vouches) and their lenders (for themstakerFrozen
andfrozenCoinAge
will be increased and staking rewards diminished).Code Snippet
If current market conditions don't allow any material withdrawal then
borrow()
still can happen andlastRepay
be set on any dust or even zero amount being lent out:UToken.sol#L611-L634
If market is such that
assetManagerContract.withdraw
can only withdraw dust or can't withdraw anything, a user can request to borrow an amount bigger than minimal, butborrow()
will be executed with some dust or even zero amount effectively borrowed. This isn't fully covered by thegetLoanableAmount()
check since it measures total funds invested viagetSupplyView()
calls to the underlying markets.As
_minBorrow
is for amount effectively borrowed, and not just for amount requested, it will be in a violation:UToken.sol#L141-L144
Also, it will have a side effect of resetting
lastRepay
even with zero amount borrowed when the borrower had no debt as of time of the call. This will effectively mark a borrower as an overdue when time since they obtained any material debt is in fact much less thanoverdueTime
:UToken.sol#L459-L465
UToken.sol#L450-L452
This can happen as subsequent
borrow()
calls will not setlastRepay
as the logic is based on having emptylastRepay
:UToken.sol#L627-L629
Tool used
Manual Review
Recommendation
Consider controlling the effective amount being borrowed, e.g.:
UToken.sol#L611-L634