hyh - SGL and BB repay do not round up both on allowance spending and elastic amount

[sherlock-admin3]

hyh

high

SGL and BB repay do not round up both on allowance spending and elastic amount

Summary

The shares to amount translation isn't done in protocol favor on BB/SGL repay.

Vulnerability Detail

Amount provided by the user can be less than shares being written off on debt repayment.

Impact

Protocol can be exploited by paying out very little amount many times, when the absence of rounding up becomes material. The impact is up to closing the debt for free.

Code Snippet

SGL repay do not round up both on allowance spending and elastic amount for the given base amount:

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/singularity/SGLLendingCommon.sol#L91-L107

    function _repay(address from, address to, bool skim, uint256 part) internal returns (uint256 amount) {
        if (part > userBorrowPart[to]) {
            part = userBorrowPart[to];
        }
        if (part == 0) revert NothingToRepay();

        if (msg.sender != from) {
            uint256 partInAmount;
            Rebase memory _totalBorrow = totalBorrow;
>>          (_totalBorrow, partInAmount) = _totalBorrow.sub(part, false);

            uint256 allowanceShare =
                _computeAllowanceAmountInAsset(to, exchangeRate, partInAmount, _safeDecimals(asset));
            if (allowanceShare == 0) revert AllowanceNotValid();
            _allowedBorrow(from, allowanceShare);
        }
>>      (totalBorrow, amount) = totalBorrow.sub(part, false);

BB repay correctly reduces allowance, but doesn't round up the elastic amount for the given base amount:

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/bigBang/BBLendingCommon.sol#L107-L126

    function _repay(address from, address to, uint256 part) internal returns (uint256 amount) {
        if (part > userBorrowPart[to]) {
            part = userBorrowPart[to];
        }
        if (part == 0) revert NothingToRepay();

        // @dev check allowance
        if (msg.sender != from) {
            uint256 partInAmount;
            Rebase memory _totalBorrow = totalBorrow;
>>          (_totalBorrow, partInAmount) = _totalBorrow.sub(part, false);
            uint256 allowanceShare =
                _computeAllowanceAmountInAsset(to, exchangeRate, partInAmount, _safeDecimals(asset));
            if (allowanceShare == 0) revert AllowanceNotValid();
            _allowedBorrow(from, allowanceShare);
        }

        // @dev sub `part` of totalBorrow
>>      (totalBorrow, amount) = totalBorrow.sub(part, true);
        userBorrowPart[to] -= part;

Tool used

Manual Review

Recommendation

Consider using true for amount calculations in all this cases, rounding the amounts up for the shares given.

[maarcweiss]

This is an interesting one. Did some research and if you take a look to all the Cualdrons, degenBox from Abracadabra (recently exploited) they indeed round up in repay. You can check all the examples at: https://www.codeslaw.app/search?chain=ethereum&q=totalBorrow.sub%28part

I will give the last word to @cryptotechmaker @0xRektora on this one, but seems true. Additionally, if possible a PoC for this rounding issues would be fantastic (to assess severity too as different roundings have different levels of loss of funds), but I guess the submitter can wait until the team decision has been taken, though leading towards valid.

[cryptotechmaker]

Yes, it seems true, but I would like to suggest for a PoC as well. Thanks! The reason for the PoC (even if it's sounds feasible) is because this code was already covered by another audit and it had a similar rounding issue submitted but this part was not included.

[cryptotechmaker]

What status should we assign to it until then @maarcweiss ?

[dmitriia]

This looks like a consequence of the mitigation changes being too general. See p.4 of Alex's Spearbit review issue (5.3.34 in public report) and mitigation commit part that wasn't needed. The reason is that rounding amount up wasn't forgiving as this amount was to be paid by the repaying user, while part being written off was fixed. I.e. while the idea of that issue was correct, these particular repayment code parts listed here indeed represent the standard rounding up of the amount due in the favor of the protocol and need to remain so.

POC is straightforward:

1. Repay dust part that have (, amount) = _totalBorrow.sub(part, false) substantially smaller vs rounding upwards (zero will not work due to if (allowanceShare == 0) revert AllowanceNotValid() check added earlier as a fix to a similar issue). It can be part = 1 wei, amount = 1 wei, while amount was actually 1.9 wei in a bigger precision and was rounded to be 1 wei.

2. Repeat many times over so the 1.9x worth of asset debt is fully repaid with 1x worth of asset paid.

The tx costs are the biggest barrier here: supposing attacker will pack the execution optimally there still be significant expenses with regard to amount as it have to be small as upwards rounding is adding 1 wei only. So this can be viable in L2 setting when asset being repaid is valuable enough to cover these costs.

[sherlock-admin3]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid

[sherlock-admin4]

The protocol team fixed this issue in PR/commit https://github.com/Tapioca-DAO/Tapioca-bar/pull/372.