Underflow possible in rebalance() of RebalanceManagerV3

[code423n4]

Handle

gpersoon

Vulnerability details

Impact

The function rebalance() of RebalanceManagerV3 calculates the amount of tokens received by subtracting oldBalance from the new balanceOf()
If the new balanceOf() happens to be smaller than oldBalance (due to an error or a weird situation at the exchange) then the variable "input" would be negative.
As the solidity version is 0.7.5 and input is an unsigned int, it would underflow and result in a very large value.

Assuming this happens at the last cycle of the inner for loop, then the following require would not revert because input is very large (although it should have reverted)

require(trade.minimumReturn <= input, "INSUFFICIENT_OUTPUT_AMOUNT");

This way you could receive less tokens than required with a revert, which is unwanted.

Proof of Concept

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/callManagers/RebalanceManagerV3.sol#L158-L206

pragma solidity ^0.7.5;
...
function rebalance(UnderlyingTrade[] calldata _swapsV2, uint256 _deadline)  external  override  onlyRebalanceManager  {
     ...
            for (uint256 j; j < trade.swaps.length; j++) {
            ...
              uint256 input = trade.quantity;
              ...
              for (uint256 j; j < trade.swaps.length; j++) {
               ...
               uint256 oldBalance = IERC20(targetToken).balanceOf(  address(basket)  );
                ...
                //The output of this trade is the input for the next trade
                input =  IERC20(targetToken).balanceOf(address(basket)) -  oldBalance;   // no safemath is used here
            }
            require(trade.minimumReturn <= input, "INSUFFICIENT_OUTPUT_AMOUNT");
          }  

Tools Used

Recommended Mitigation Steps

Use safemath for the subtraction

[MiniRoman]

If the new balanceOf() happens to be smaller than oldBalance (due to an error or a weird situation at the exchange)

This scenario is not really possible. After buying a token, it's rather impossible, that it's balance will be smaller after buying it, then before the swap.

https://github.com/code-423n4/2021-12-amun/blob/cf890dedf2e43ec787e8e5df65726316fda134a1/contracts/basket/contracts/callManagers/RebalanceManagerV3.sol#L181

[0xleastwood]

This issue would involve the rebalance manager setting a malicious or improper exchange which is approved to transfer out the target token. As the sponsor has stated, it is extremely unlikely that an exchange will become malicious and transfer out the target token. However, I think it is still worth considering as a preventative measure. Marking this as low.