Merge pull request #229 from Ithil-protocol/audit-fix-i8

fix i8 and i9

src/Constants.sol

@@ -3,3 +3,5 @@ pragma solidity =0.8.18;
 
 uint256 constant RESOLUTION = 1e18;
 uint256 constant ONE_YEAR = 365 days;
+uint256 constant MINIMUM_FEE_UNLOCK_TIME = 30 seconds;
+uint256 constant MAXIMUM_FEE_UNLOCK_TIME = 7 days;

src/Vault.sol

@@ -8,6 +8,7 @@ import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.s
 import { ERC20, ERC20Permit } from "@openzeppelin/contracts/token/ERC20/extensions/draft-ERC20Permit.sol";
 import { ERC4626, IERC4626 } from "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol";
 import { IVault } from "./interfaces/IVault.sol";
+import { MINIMUM_FEE_UNLOCK_TIME, MAXIMUM_FEE_UNLOCK_TIME } from "./Constants.sol";
 
 // Since this vault inherits from ERC4626, it has the known vulnerability of the "donation attack"
 // In the Ithil framework this is fixed by deploying the Vault already with 1 token deposited
@@ -58,9 +59,8 @@ contract Vault is IVault, ERC4626, ERC20Permit {
     }
 
     function setFeeUnlockTime(uint256 _feeUnlockTime) external override onlyOwner {
-        // Minimum 30 seconds, maximum 7 days
-        // This also avoids division by zero in _calculateLockedProfits()
-        if (_feeUnlockTime < 30 seconds || _feeUnlockTime > 7 days) revert FeeUnlockTimeOutOfRange();
+        if (_feeUnlockTime < MINIMUM_FEE_UNLOCK_TIME || _feeUnlockTime > MAXIMUM_FEE_UNLOCK_TIME)
+            revert FeeUnlockTimeOutOfRange();
         currentProfits = _calculateLockedProfits();
         currentLosses = _calculateLockedLosses();
         latestRepay = block.timestamp;
@@ -230,7 +230,7 @@ contract Vault is IVault, ERC4626, ERC20Permit {
         currentLosses = _calculateLockedLosses() + (assets - loan);
         latestRepay = block.timestamp;
 
-        IERC20(asset()).safeTransfer(receiver, assets);
+        if (assets > 0) IERC20(asset()).safeTransfer(receiver, assets);
 
         emit Borrowed(receiver, assets);
 
@@ -265,7 +265,7 @@ contract Vault is IVault, ERC4626, ERC20Permit {
         // the vault is not responsible for any payoff
         // slither-disable-next-line arbitrary-send-erc20
         // super.totalAssets() += assets and never overflows by definition
-        IERC20(asset()).safeTransferFrom(repayer, address(this), assets);
+        if (assets > 0) IERC20(asset()).safeTransferFrom(repayer, address(this), assets);
 
         emit Repaid(repayer, assets, debt);
     }

src/irmodels/AuctionRateModel.sol

@@ -9,6 +9,7 @@ import { BaseRiskModel } from "../services/BaseRiskModel.sol";
 /// Rate model in which baseIR is based on a Dutch auction
 /// 1e18 corresponds to 1, i.e. an interest rate of 100%
 abstract contract AuctionRateModel is Ownable, BaseRiskModel {
+    uint256 private constant MAX_RATE = 1e18;
     /**
      * @dev gas saving trick
      * latest is a timestamp and base < 1e18, they all fit in uint256
@@ -25,7 +26,8 @@ abstract contract AuctionRateModel is Ownable, BaseRiskModel {
     error ZeroMarginLoan();
 
     function setRiskParams(address token, uint256 riskSpread, uint256 baseRate, uint256 halfTime) external onlyOwner {
-        if (token == address(0) || baseRate > 1e18 || riskSpread > 1e18 || halfTime == 0) revert InvalidInitParams();
+        if (token == address(0) || baseRate > MAX_RATE || riskSpread > MAX_RATE || halfTime == 0)
+            revert InvalidInitParams();
         riskSpreads[token] = riskSpread;
         latestAndBase[token] = (block.timestamp << 128) + baseRate;
         halvingTime[token] = halfTime;
@@ -74,7 +76,7 @@ abstract contract AuctionRateModel is Ownable, BaseRiskModel {
             freeLiquidity
         );
         // Reset new base and latest borrow, force IR stays below resolution
-        if (newBase + spread > 1e18) revert InterestRateOverflow();
+        if (newBase + spread > MAX_RATE) revert InterestRateOverflow();
         latestAndBase[loan.token] = (block.timestamp << 128) + newBase;
 
         return (newBase, spread);

src/services/credit/CallOption.sol

@@ -8,6 +8,7 @@ import { Service } from "../Service.sol";
 import { IService } from "../../interfaces/IService.sol";
 import { IVault } from "../../interfaces/IVault.sol";
 import { Service } from "../Service.sol";
+import { RESOLUTION } from "../../Constants.sol";
 
 /// @title    Call option contract
 /// @author   Ithil
@@ -173,14 +174,15 @@ contract CallOption is CreditService {
         // We also add a slippage check to protect signers from liquidity crunch attacks
         (uint256 calledPortion, uint256 minRedeemed) = abi.decode(data, (uint256, uint256));
         address ownerAddress = ownerOf(tokenID);
-        if (calledPortion > 1e18 || (msg.sender != ownerAddress && calledPortion > 0)) revert InvalidCalledPortion();
+        if (calledPortion > RESOLUTION || (msg.sender != ownerAddress && calledPortion > 0))
+            revert InvalidCalledPortion();
 
         uint256 toBorrow;
         uint256 freeLiquidity;
         // redeem mechanism
         IVault vault = IVault(manager.vaults(agreement.loans[0].token));
         // calculate the amount of shares to redeem to get dueAmount
-        uint256 toCall = (agreement.collaterals[1].amount * calledPortion) / 1e18;
+        uint256 toCall = (agreement.collaterals[1].amount * calledPortion) / RESOLUTION;
         // The amount of ithil not called can be added back to the total allocation
         totalAllocation += (agreement.collaterals[1].amount - toCall);
         uint256 toTransfer = dueAmount(agreement, data);
@@ -207,7 +209,7 @@ contract CallOption is CreditService {
             toBorrow = toTransfer - redeemed > freeLiquidity ? freeLiquidity : toTransfer - redeemed;
         }
         // We will always have ithil.balanceOf(address(this)) >= toCall, so the following succeeds
-        ithil.safeTransfer(ownerAddress, toCall);
+        if (toCall > 0) ithil.safeTransfer(ownerAddress, toCall);
         // repay the user's losses
         if (toBorrow > 0 && freeLiquidity > 0) manager.borrow(agreement.loans[0].token, toBorrow, 0, ownerAddress);
     }
@@ -228,7 +230,7 @@ contract CallOption is CreditService {
         (uint256 calledPortion, ) = abi.decode(data, (uint256, uint256));
 
         // The non-called portion is capital to give back to the user
-        return (agreement.loans[0].amount * (1e18 - calledPortion)) / 1e18;
+        return (agreement.loans[0].amount * (RESOLUTION - calledPortion)) / RESOLUTION;
     }
 
     function allocateIthil(uint256 amount) external {

src/services/debit/GmxService.sol

@@ -19,6 +19,8 @@ import { Service } from "../Service.sol";
 contract GmxService is AuctionRateModel, DebitService {
     using SafeERC20 for IERC20;
 
+    uint256 private constant BASIS_POINTS_DIVISOR = 10000;
+
     IRewardRouter public immutable router;
     IRewardRouterV2 public immutable routerV2;
     IERC20 public immutable glp;
@@ -117,7 +119,7 @@ contract GmxService is AuctionRateModel, DebitService {
         totalRewards = toTransfer < newRewards ? newRewards - toTransfer : 0;
         totalCollateral -= agreement.collaterals[0].amount;
         // Transfer weth: since toTransfer <= totalWithdraw
-        weth.safeTransfer(msg.sender, toTransfer);
+        if (toTransfer > 0) weth.safeTransfer(msg.sender, toTransfer);
     }
 
     function wethReward(uint256 tokenID) public view returns (uint256) {
@@ -151,7 +153,7 @@ contract GmxService is AuctionRateModel, DebitService {
             usdgVault.taxBasisPoints(),
             false
         );
-        results[0] = (usdgDelta * (10000 - feeBasisPoints)) / 10000;
+        results[0] = (usdgDelta * (BASIS_POINTS_DIVISOR - feeBasisPoints)) / BASIS_POINTS_DIVISOR;
 
         return results;
     }