BigBang/Singularity::sellCollateral - Surplus of collateral with regards to repay amount is never returned to user

[code423n4]

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L394 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L417-L419

Vulnerability details

Impact

In both Singularity and BigBang, the remainder of removed collateral is never returned to the user in sellCollateral after repayment of the loan

Proof of Concept

A user can call sellCollateral with a bigger share than the amount she borrowed.

The whole collateral share is first removed:

_removeCollateral(from, address(swapper), share);

The swap determines the shareOut value, and then this condition is executed:

if (shareOwed <= shareOut) {
    _repay(from, from, partOwed);
}

Which means that only the part borrowed by the user is repaid, and the rest of asset obtained after swap only stays in BigBang. This means that the user actually lost the remainder of what was swapped, and the call does not revert unless the user is not solvent

Tools Used

Manual review

Recommended Mitigation Steps

Add the surplus of asset to the balance of the user selling collateral

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-sponsor]

0xRektora marked the issue as sponsor confirmed

[c4-judge]

dmvt marked the issue as selected for report

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[dmvt]

Downgrading to medium because the amounts leaked / lost in the two reports are almost always very small.

[JeffCX]

Thanks for judging the contest!

I agree with the judge's comments

the amounts leaked / lost in the two reports are almost always very small.

but I think whether the unhandled amount is small depends on slippage + the leak of value can always happen

it is possible that in one transaction, a tiny amount of Surplus of collateral is not refunded, but in 10000 transaction, all surplus does not refund to user and the amount add up

so the leak value is relatively large as more user use the application

so I politely think the severity is high

I respect judge's final decision and will have no further dispute!

[dmvt]

I disagree. The leak would almost certainly be noticed and the contract replaced before the values got very high. Medium stands.