From 3fc1a4df5cf6280e61ce890514fb82f26576787e Mon Sep 17 00:00:00 2001 From: paul4912 Date: Wed, 15 Jun 2022 12:14:53 +1000 Subject: [PATCH] fix audit changes --- contracts/interfaces/ICurvePoolFactory.sol | 3 ++ contracts/interfaces/IIncurDebt.sol | 6 +++ contracts/interfaces/IStrategy.sol | 1 + contracts/peripheral/IncurDebt.sol | 27 +++++++--- contracts/peripheral/Strategies/UniSwap.sol | 29 +++++------ test/debt/IncurDebt.js | 55 ++++++++++----------- 6 files changed, 69 insertions(+), 52 deletions(-) diff --git a/contracts/interfaces/ICurvePoolFactory.sol b/contracts/interfaces/ICurvePoolFactory.sol index aad4f9481..c68a96d01 100644 --- a/contracts/interfaces/ICurvePoolFactory.sol +++ b/contracts/interfaces/ICurvePoolFactory.sol @@ -1,3 +1,6 @@ +// SPDX-License-Identifier: AGPL-3.0 +pragma solidity ^0.8.10; + interface ICurvePoolFactory { function get_coins(address _pool) external view returns (address[4] memory); diff --git a/contracts/interfaces/IIncurDebt.sol b/contracts/interfaces/IIncurDebt.sol index de09e30e1..de3df1957 100644 --- a/contracts/interfaces/IIncurDebt.sol +++ b/contracts/interfaces/IIncurDebt.sol @@ -116,4 +116,10 @@ interface IIncurDebt { * @param _borrower the account to seize */ function seize(address _borrower) external; + + /// @notice lets governor withdraw tokens incase of airdrop or error + /// - onlyOwner (or governance) + /// @param _tokenAddress the address of the token + /// @param _amount amount of tokens to withdraw + function withdrawToken(address _tokenAddress, uint256 _amount) external; } diff --git a/contracts/interfaces/IStrategy.sol b/contracts/interfaces/IStrategy.sol index 7edcb2ae5..5b0b5168f 100644 --- a/contracts/interfaces/IStrategy.sol +++ b/contracts/interfaces/IStrategy.sol @@ -1,3 +1,4 @@ +// SPDX-License-Identifier: AGPL-3.0 pragma solidity ^0.8.10; /** diff --git a/contracts/peripheral/IncurDebt.sol b/contracts/peripheral/IncurDebt.sol index 9e842dea9..f0dc0cd7d 100644 --- a/contracts/peripheral/IncurDebt.sol +++ b/contracts/peripheral/IncurDebt.sol @@ -22,6 +22,7 @@ error IncurDebt_AmountAboveBorrowerBalance(uint256 _amount); error IncurDebt_OHMAmountMoreThanAvailableLoan(uint256 _amount); error IncurDebt_BorrowerHasNoOutstandingDebt(address _borrower); error IncurDebt_BorrowerStillHasOutstandingDebt(address _borrower); +error IncurDebt_InvalidAddress(); /// @title IncurDebt /// @notice Contract that allows users to use the treasury's incurdebt function. @@ -107,6 +108,13 @@ contract IncurDebt is OlympusAccessControlledV2, IIncurDebt { address _treasury, address _olympusAuthority ) OlympusAccessControlledV2(IOlympusAuthority(_olympusAuthority)) { + if (_OHM == address(0)) revert IncurDebt_InvalidAddress(); + if (_gOHM == address(0)) revert IncurDebt_InvalidAddress(); + if (_sOHM == address(0)) revert IncurDebt_InvalidAddress(); + if (_staking == address(0)) revert IncurDebt_InvalidAddress(); + if (_treasury == address(0)) revert IncurDebt_InvalidAddress(); + if (_olympusAuthority == address(0)) revert IncurDebt_InvalidAddress(); + OHM = _OHM; gOHM = _gOHM; sOHM = _sOHM; @@ -151,7 +159,6 @@ contract IncurDebt is OlympusAccessControlledV2, IIncurDebt { /// @notice lets a user become a LP borrower /// - onlyOwner (or governance) - /// - user must not be borrower /// @param _borrower the address that will interact with contract function allowLPBorrower(address _borrower) external override onlyGovernor { if (borrowers[_borrower].isNonLpBorrower) revert IncurDebt_AlreadyBorrower(_borrower); @@ -163,7 +170,6 @@ contract IncurDebt is OlympusAccessControlledV2, IIncurDebt { /// @notice lets a user become a Non LP borrower /// - onlyOwner (or governance) - /// - user must not be borrower /// @param _borrower the address that will interact with contract function allowNonLPBorrower(address _borrower) external override onlyGovernor { if (borrowers[_borrower].isLpBorrower) revert IncurDebt_AlreadyBorrower(_borrower); @@ -176,7 +182,6 @@ contract IncurDebt is OlympusAccessControlledV2, IIncurDebt { /// @notice sets the maximum debt limit for a borrower /// - onlyOwner (or governance) /// - limit must be greater than or equal to borrower's outstanding debt - /// - limit must be less than or equal to the global debt limit /// @param _borrower the address that will interact with contract /// @param _limit borrower's debt limit in OHM function setBorrowerDebtLimit(address _borrower, uint256 _limit) @@ -227,7 +232,7 @@ contract IncurDebt is OlympusAccessControlledV2, IIncurDebt { /// - will burn all collateral, including excess of debt /// - onlyGovernance /// @param _borrower the account to seize collateral - function seize(address _borrower) external override onlyGovernor isBorrower(_borrower) { + function seize(address _borrower) external override onlyGovernor { (uint256 seizedCollateral, uint256 paidDebt) = _repay(_borrower); borrowers[_borrower].collateralInGOHM = 0; @@ -237,6 +242,14 @@ contract IncurDebt is OlympusAccessControlledV2, IIncurDebt { emit DebtPaidWithCollateralAndBurnTheRest(_borrower, paidDebt, totalOutstandingGlobalDebt, seizedCollateral); } + /// @notice lets governor withdraw tokens incase of airdrop or error + /// - onlyOwner (or governance) + /// @param _tokenAddress the address of the token + /// @param _amount amount of tokens to withdraw + function withdrawToken(address _tokenAddress, uint256 _amount) external override onlyGovernor { + IERC20(_tokenAddress).safeTransfer(msg.sender, _amount); + } + /************************ * Borrower Functions ************************/ @@ -379,7 +392,6 @@ contract IncurDebt is OlympusAccessControlledV2, IIncurDebt { revert IncurDebt_AmountAboveBorrowerBalance(_amount); if (_amount > borrower.collateralInGOHM - borrower.unwrappedGOHM) { - // Does uint256 > uint128 cause problem? uint256 amountGOHMToWrap = borrower.unwrappedGOHM + _amount - borrower.collateralInGOHM; borrower.unwrappedGOHM -= uint128(amountGOHMToWrap); uint256 amountOHMNeededToWrap = IgOHM(gOHM).balanceFrom(amountGOHMToWrap); @@ -413,7 +425,7 @@ contract IncurDebt is OlympusAccessControlledV2, IIncurDebt { IERC20(gOHM).transfer(msg.sender, collateralRemaining); - emit DebtPaidWithCollateralAndWithdrawTheRest( //Change event + emit DebtPaidWithCollateralAndWithdrawTheRest( msg.sender, paidDebt, totalOutstandingGlobalDebt, @@ -430,6 +442,9 @@ contract IncurDebt is OlympusAccessControlledV2, IIncurDebt { Borrower storage borrower = borrowers[msg.sender]; if (borrower.debt == 0) revert IncurDebt_BorrowerHasNoOutstandingDebt(msg.sender); + if (_ohmAmount > borrower.debt) { + _ohmAmount = borrower.debt; + } totalOutstandingGlobalDebt -= _ohmAmount; borrower.debt -= uint128(_ohmAmount); diff --git a/contracts/peripheral/Strategies/UniSwap.sol b/contracts/peripheral/Strategies/UniSwap.sol index 87a0db67f..f64e63f6f 100644 --- a/contracts/peripheral/Strategies/UniSwap.sol +++ b/contracts/peripheral/Strategies/UniSwap.sol @@ -60,9 +60,8 @@ contract UniSwapStrategy is IStrategy { uint256 amountADesired, uint256 amountBDesired, uint256 amountAMin, - uint256 amountBMin, - uint256 slippage - ) = abi.decode(_data, (address, address, uint256, uint256, uint256, uint256, uint256)); + uint256 amountBMin + ) = abi.decode(_data, (address, address, uint256, uint256, uint256, uint256)); if (tokenA == ohmAddress) { if (_ohmAmount != amountADesired) revert UniswapStrategy_AmountDoesNotMatch(); @@ -74,8 +73,8 @@ contract UniSwapStrategy is IStrategy { if (_ohmAmount != amountBDesired) revert UniswapStrategy_AmountDoesNotMatch(); IERC20(tokenB).safeTransferFrom(incurDebtAddress, address(this), _ohmAmount); - IERC20(tokenA).safeTransferFrom(_user, address(this), amountBDesired); - IERC20(tokenA).approve(address(router), amountBDesired); + IERC20(tokenA).safeTransferFrom(_user, address(this), amountADesired); + IERC20(tokenA).approve(address(router), amountADesired); } else { revert UniswapStrategy_OhmAddressNotFound(); } @@ -88,8 +87,8 @@ contract UniSwapStrategy is IStrategy { tokenB, amountADesired, amountBDesired, - (amountAMin * slippage) / 1000, - (amountBMin * slippage) / 1000, + amountAMin, + amountBMin, incurDebtAddress, block.timestamp ); @@ -128,14 +127,10 @@ contract UniSwapStrategy is IStrategy { address _user ) external returns (uint256 ohmRecieved) { if (msg.sender != incurDebtAddress) revert UniswapStrategy_NotIncurDebtAddress(); - ( - address tokenA, - address tokenB, - uint256 liquidity, - uint256 amountAMin, - uint256 amountBMin, - uint256 slippage - ) = abi.decode(_data, (address, address, uint256, uint256, uint256, uint256)); + (address tokenA, address tokenB, uint256 liquidity, uint256 amountAMin, uint256 amountBMin) = abi.decode( + _data, + (address, address, uint256, uint256, uint256) + ); address lpTokenAddress = IUniswapV2Factory(factory).getPair(tokenA, tokenB); @@ -149,8 +144,8 @@ contract UniSwapStrategy is IStrategy { tokenA, tokenB, liquidity, - (amountAMin * slippage) / 1000, - (amountBMin * slippage) / 1000, + amountAMin, + amountBMin, address(this), block.timestamp ); diff --git a/test/debt/IncurDebt.js b/test/debt/IncurDebt.js index c7e7b80a5..8ea698ad0 100644 --- a/test/debt/IncurDebt.js +++ b/test/debt/IncurDebt.js @@ -725,11 +725,10 @@ describe("IncurDebt", async () => { const daiAmount = "1000000000000000000000"; const token0 = olympus.ohm; const token1 = "0x6B175474E89094C44Da98b954EedeAC495271d0F"; - const slippage = 900; const data = ethers.utils.defaultAbiCoder.encode( - ["address", "address", "uint256", "uint256", "uint256", "uint256", "uint256"], - [token0, token1, ohmAmount, daiAmount, ohmAmount, daiAmount, slippage] + ["address", "address", "uint256", "uint256", "uint256", "uint256"], + [token0, token1, ohmAmount, daiAmount, "30000000000", "100000000000000000000"] ); it("Should fail if borrower isNonLpBorrower", async () => { @@ -1010,17 +1009,14 @@ describe("IncurDebt", async () => { const token0 = olympus.ohm; const token1 = "0x6B175474E89094C44Da98b954EedeAC495271d0F"; - const slippage = 900; - const slippage1 = 900; - const fakeData = ethers.utils.defaultAbiCoder.encode( - ["address", "address", "uint256", "uint256", "uint256", "uint256"], - [token0, token1, ohmAmount, daiAmount, ohmAmount, slippage] + ["address", "address", "uint256", "uint256", "uint256"], + [token0, token1, ohmAmount, daiAmount, ohmAmount] ); const data1 = ethers.utils.defaultAbiCoder.encode( - ["address", "address", "uint256", "uint256", "uint256", "uint256", "uint256"], - [token0, token1, ohmAmount, daiAmount, ohmAmount, daiAmount, slippage] + ["address", "address", "uint256", "uint256", "uint256", "uint256"], + [token0, token1, ohmAmount, daiAmount, "30000000000", "100000000000000000000"] ); it("Should fail if borrower isNonLpBorrower", async () => { @@ -1068,23 +1064,12 @@ describe("IncurDebt", async () => { const totalOutstandingGlobalDebtBeforeTx = await incurDebt.totalOutstandingGlobalDebt(); assert.equal(borrowerInfoBeforeTx.debt, totalOutstandingGlobalDebtBeforeTx.toString()); - const token0PoolBalance = await ohm_token.balanceOf(uniOhmDaiLpAddress); - const token1PoolBalance = await daiContract.balanceOf(uniOhmDaiLpAddress); - const poolTotalSupply = await uniswapLpContract.totalSupply(); - - const amount1Min = (token0PoolBalance * borrowerLpBeforeTx) / poolTotalSupply; - const amount2Min = (token1PoolBalance * borrowerLpBeforeTx) / poolTotalSupply; + const amount1Min = "30000000000"; + const amount2Min = "100000000000000000000"; const data = ethers.utils.defaultAbiCoder.encode( - ["address", "address", "uint256", "uint256", "uint256", "uint256"], - [ - token0, - token1, - borrowerLpBeforeTx, - amount1Min.toString(), - amount2Min.toString(), - slippage1, - ] + ["address", "address", "uint256", "uint256", "uint256"], + [token0, token1, borrowerLpBeforeTx, amount1Min, amount2Min] ); await expect( incurDebt @@ -1240,11 +1225,9 @@ describe("IncurDebt", async () => { const token0 = olympus.ohm; const token1 = "0x6B175474E89094C44Da98b954EedeAC495271d0F"; - const slippage = 900; - const data = ethers.utils.defaultAbiCoder.encode( - ["address", "address", "uint256", "uint256", "uint256", "uint256", "uint256"], - [token0, token1, ohmAmount, daiAmount, ohmAmount, daiAmount, slippage] + ["address", "address", "uint256", "uint256", "uint256", "uint256"], + [token0, token1, ohmAmount, daiAmount, "30000000000", "100000000000000000000"] ); it("Should fail if borrower isNonLpBorrower", async () => { @@ -1390,6 +1373,20 @@ describe("IncurDebt", async () => { }); }); + describe("Withdraw Tokens as Governor", async () => { + it("Should be able to sweep tokens", async () => { + await daiContract + .connect(daiHolder) + .transfer(incurDebt.address, "10000000000000000000"); + let before = await daiContract.balanceOf(governor.address); + await incurDebt + .connect(governor) + .withdrawToken(daiContract.address, "10000000000000000000"); + let after = await daiContract.balanceOf(governor.address); + await expect(after.sub(before)).to.equal("10000000000000000000"); + }); + }); + async function impersonate(address) { await impersonateAccount(address); const owner = await ethers.getSigner(address);