code-423n4 / 2021-10-union-findings

0 stars 0 forks source link

AssetManager: Deposit() function has redundant continue statement. #29

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

itsmeSTYJ

Vulnerability details

Impact

Gas op

Proof of Concept

/**
 *  @dev Deposit tokens to AssetManager, and those tokens will be passed along to adapters to deposit to integrated asset protocols if any is available.
 *  @param token ERC20 token address
 *  @param amount ERC20 token address
 *  @return Deposited amount
 */
function deposit(address token, uint256 amount)
    external
    override
    whenNotPaused
    onlyAuth(token)
    nonReentrant
    returns (bool)
{
    IERC20Upgradeable poolToken = IERC20Upgradeable(token);
    require(amount > 0, "AssetManager: amount can not be zero");

    if (!_isUToken(msg.sender, token)) {
        balances[msg.sender][token] += amount;
        totalPrincipal[token] += amount;
    }

    bool remaining = true;
    if (isMarketSupported(token)) {
        // assumption: markets are arranged in order of decreasing liquidity
        // iterate markets till floors are filled
        // floors define minimum amount to maintain confidence in liquidity
        for (uint256 i = 0; i < moneyMarkets.length && remaining; i++) {
            IMoneyMarketAdapter moneyMarket = moneyMarkets[i];

            if (!moneyMarket.supportsToken(token)) continue;
            if (moneyMarket.floorMap(token) <= moneyMarket.getSupply(token)) continue;

            poolToken.safeTransferFrom(msg.sender, address(moneyMarket), amount);
            moneyMarket.deposit(token);
            remaining = false;
        }

        // assumption: less liquid markets provide more yield
        // iterate markets in reverse to optimize for yield
        // do this only if floors are filled i.e. min liquidity satisfied
        // dposit in the market where ceiling is not being exceeded
        for (uint256 j = moneyMarkets.length; j > 0 && remaining; j--) {
            IMoneyMarketAdapter moneyMarket = moneyMarkets[j - 1];
            if (!moneyMarket.supportsToken(token)) continue;

            uint256 supply = moneyMarket.getSupply(token);
            uint256 ceiling = moneyMarket.ceilingMap(token);
            // if (ceiling <= supply) continue; // deleted this
            if (supply + amount > ceiling) continue;

            poolToken.safeTransferFrom(msg.sender, address(moneyMarket), amount);
            moneyMarket.deposit(token);
            remaining = false;
        }
    }

    if (remaining) {
        poolToken.safeTransferFrom(msg.sender, address(this), amount);
    }

    emit LogDeposit(token, msg.sender, amount);

    return true;
}

The if (ceiling <= supply) continue; is unnecessary as the next check if (supply + amount > ceiling) continue; covers both. The second check can be rewritten as if (ceiling <= supply + amount) continue; so deposits can only happens if the ceiling is greater than total supply + the amount to be deposited.

GalloDaSballo commented 2 years ago

Agree that the first check is unnecessary

Am not sure the refactoring is a good idea as it seems more confusing to me