sherlock-audit / 2022-10-union-finance-judging

4 stars 1 forks source link

Ch_301 - Users will receive less than they expect from the `borrow()` #72

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

Ch_301

medium

Users will receive less than they expect from the borrow()

Summary

Alice will invoke borrow() with amount == X He will receive only X - n But updateLocked() will update locked with X value

Vulnerability Detail

On UToken.sol ==> borrow()

if (!assetManagerContract.withdraw(underlying, to, amount)) revert WithdrawFailed();

On AssetManager.sol ==> withdraw() in case isMarketSupported(token) is true This loop will work

            for (uint256 i = 0; i < withdrawSeqLength && remaining > 0; i++) {
                IMoneyMarketAdapter moneyMarket = moneyMarkets[withdrawSeq[i]];
                if (!moneyMarket.supportsToken(token)) continue;

                uint256 supply = moneyMarket.getSupply(token);
                if (supply == 0) continue;

                uint256 withdrawAmount = supply < remaining ? supply : remaining;
                remaining -= withdrawAmount;
                moneyMarket.withdraw(token, account, withdrawAmount);
            }

in case i == withdrawSeqLength and remaining > 0 this loop will stop. And there is no way to complete sending the remaining balace The withdraw() will return True even if the remaining > 0

Now go back to UToken.sol ==> borrow()

        if (!assetManagerContract.withdraw(underlying, to, amount)) revert WithdrawFailed();

        IUserManager(userManager).updateLocked(msg.sender, uint96(amount + fee), true);

The first check will passed successfully And the second line will update the locked amount to amount + fee even if the user dosen’t receive all the amount

Impact

The user will receive less amount of the locked

Code Snippet

          if (isMarketSupported(token)) {
            uint256 withdrawSeqLength = withdrawSeq.length;
            // iterate markets according to defined sequence and withdraw
            for (uint256 i = 0; i < withdrawSeqLength && remaining > 0; i++) {
                IMoneyMarketAdapter moneyMarket = moneyMarkets[withdrawSeq[i]];
                if (!moneyMarket.supportsToken(token)) continue;

                uint256 supply = moneyMarket.getSupply(token);
                if (supply == 0) continue;

                uint256 withdrawAmount = supply < remaining ? supply : remaining;
                remaining -= withdrawAmount;
                moneyMarket.withdraw(token, account, withdrawAmount);
            }
        }

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/asset/AssetManager.sol#L345-L359

        if (!assetManagerContract.withdraw(underlying, to, amount)) revert WithdrawFailed();

        IUserManager(userManager).updateLocked(msg.sender, uint96(amount + fee), true);

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/market/UToken.sol#L547-L552

Tool used

Manual Review

Recommendation

You can check remaining on withdraw()

duplicate of #27

kingjacob commented 1 year ago

dupe of #27