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

0 stars 0 forks source link

AssetManager: rebalance() function can optimized further. #30

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

itsmeSTYJ

Vulnerability details

Impact

Leaving out the last percentage when calling rebalance() serves no purpose and should be removed. By removing, you will also be able to save some gas since you're no longer required to do the conditional check.

Proof of Concept

function rebalance(address tokenAddress, uint256[] calldata percentages)
    external
    override
    checkMarketSupported(tokenAddress)
    onlyAdmin
{
    IERC20Upgradeable token = IERC20Upgradeable(tokenAddress);
    // require(percentages.length + 1 == moneyMarkets.length, "AssetManager: percentages error"); // deleted this
        require(percentages.length == moneyMarkets.length, "AssetManager: percentages error"); // added this

    for (uint256 i = 0; i < moneyMarkets.length; i++) {
        if (!moneyMarkets[i].supportsToken(tokenAddress)) {
            continue;
        }
        moneyMarkets[i].withdrawAll(tokenAddress, address(this));
    }

    uint256 tokenSupply = token.balanceOf(address(this));

    for (uint256 i = 0; i < percentages.length; i++) {
        if (!moneyMarkets[i].supportsToken(tokenAddress)) {
            continue;
        }
        uint256 amountToDeposit = (tokenSupply * percentages[i]) / 10000;
        if (amountToDeposit == 0) {
            continue;
        }
        token.safeTransfer(address(moneyMarkets[i]), amountToDeposit);
        moneyMarkets[i].deposit(tokenAddress);
    }
      // Deleted the following lines
    // uint256 remainingTokens = token.balanceOf(address(this));
    // if (moneyMarkets[moneyMarkets.length - 1].supportsToken(tokenAddress) && remainingTokens > 0) {
    //     token.safeTransfer(address(moneyMarkets[moneyMarkets.length - 1]), remainingTokens);
    //     moneyMarkets[moneyMarkets.length - 1].deposit(tokenAddress);
    // }

    require(token.balanceOf(address(this)) == 0, "AssetManager: there are remaining funds in the fund pool");

    emit LogRebalance(tokenAddress, percentages);
}
GalloDaSballo commented 3 years ago

Disagree with the optimization at face value

What if there are remainign tokens? The system is meant to send them back

The optimization is not valid unless you also prove that no funds would remain at the end of rebalance