Merge pull request #1 from refresh-labs/fix/audit

audit: make all audit fixes

src/AvailDepository.sol

@@ -38,6 +38,7 @@ contract AvailDepository is PausableUpgradeable, AccessControlDefaultAdminRulesU
         if (address(newAvail) == address(0) || address(newBridge) == address(0)) revert ZeroAddress();
         avail = newAvail;
         bridge = newBridge;
+        _disableInitializers();
     }
 
     /// @notice Initializes the AvailDepository contract with governance, bridge, depositor, and depository
@@ -83,10 +84,15 @@ contract AvailDepository is PausableUpgradeable, AccessControlDefaultAdminRulesU
     /// @notice Deposits Avail ERC20 to the depository on Avail
     /// @dev Reverts if the sender is not the depositor
     function deposit() external whenNotPaused onlyRole(DEPOSITOR_ROLE) {
-        uint256 amount = avail.balanceOf(address(this));
+        uint256 amount = avail.balanceOf(address(this)) - 1;
+        emit Deposit(amount);
         // keep 1 wei so slot stays warm, intentionally leave return unused, since OZ impl does not return false
         // slither-disable-next-line unused-return
-        avail.approve(address(bridge), amount - 1);
-        bridge.sendAVAIL(depository, amount - 1);
+        avail.approve(address(bridge), amount);
+        bridge.sendAVAIL(depository, amount);
+    }
+
+    function withdraw(IERC20 token, uint256 amount) external onlyRole(DEFAULT_ADMIN_ROLE) {
+        token.safeTransfer(msg.sender, amount);
     }
 }

src/AvailWithdrawalHelper.sol

@@ -10,10 +10,11 @@ import {
     IERC721Metadata,
     ERC721Upgradeable
 } from "lib/openzeppelin-contracts-upgradeable/contracts/token/ERC721/ERC721Upgradeable.sol";
-import {PausableUpgradeable} from "lib/openzeppelin-contracts-upgradeable/contracts/utils/PausableUpgradeable.sol";
-import {SafeERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol";
+import {Math} from "lib/openzeppelin-contracts/contracts/utils/math/Math.sol";
 import {Ownable2StepUpgradeable} from
     "lib/openzeppelin-contracts-upgradeable/contracts/access/Ownable2StepUpgradeable.sol";
+import {PausableUpgradeable} from "lib/openzeppelin-contracts-upgradeable/contracts/utils/PausableUpgradeable.sol";
+import {SafeERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol";
 
 import {IAvailWithdrawalHelper} from "src/interfaces/IAvailWithdrawalHelper.sol";
 import {IStakedAvail} from "src/interfaces/IStakedAvail.sol";
@@ -28,6 +29,7 @@ contract AvailWithdrawalHelper is
     ERC721Upgradeable,
     IAvailWithdrawalHelper
 {
+    using Math for uint256;
     using SafeERC20 for IERC20;
 
     bytes32 private constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
@@ -55,6 +57,7 @@ contract AvailWithdrawalHelper is
     constructor(IERC20 newAvail) {
         if (address(newAvail) == address(0)) revert ZeroAddress();
         avail = newAvail;
+        _disableInitializers();
     }
 
     /// @notice Initializes the AvailWithdrawalHelper contract with governance, Avail, Staked Avail, and minimum withdrawal
@@ -99,7 +102,7 @@ contract AvailWithdrawalHelper is
     }
 
     /// @notice Returns fulfilment amount between lastFulfillment and till
-    /// @dev Reverts if till is less than or equal to lastFulfillment
+    /// @dev Reverts from underflow if till is behind lastFulfillment
     /// @param till Token ID to iterate till
     function previewFulfill(uint256 till) public view returns (uint256 amount) {
         return withdrawals[till].accAmount - withdrawals[lastFulfillment].accAmount;
@@ -137,8 +140,9 @@ contract AvailWithdrawalHelper is
         address owner = ownerOf(id);
         withdrawalAmount -= amount;
         _burn(id);
-        stAvail.updateAssetsFromWithdrawals(amount, withdrawal.shares);
-        avail.safeTransfer(owner, amount);
+        uint256 returnAmt = Math.min(amount, stAvail.previewBurn(withdrawal.shares));
+        stAvail.updateAssetsFromWithdrawals(returnAmt, withdrawal.shares);
+        avail.safeTransfer(owner, returnAmt);
     }
 
     /// @notice Returns the withdrawal amount and shares in a particular ID

src/DeqRouter.sol

@@ -14,7 +14,7 @@ import {IStakedAvail} from "src/interfaces/IStakedAvail.sol";
 /// @title DeqRouter
 /// @author Deq Protocol
 /// @notice Router contract for swapping ERC20 tokens to Avail and minting staked Avail
-/// @dev The contract is upgradeable and uses AccessControlDefaultAdminRulesUpgradeable
+/// @dev The contract is upgradeable. The router does not support fee-on-transfer tokens as 0x proxy does not.
 contract DeqRouter is PausableUpgradeable, AccessControlDefaultAdminRulesUpgradeable, IDeqRouter {
     using SafeERC20 for IERC20;
 
@@ -29,6 +29,7 @@ contract DeqRouter is PausableUpgradeable, AccessControlDefaultAdminRulesUpgrade
     constructor(IERC20 newAvail) {
         if (address(newAvail) == address(0)) revert ZeroAddress();
         avail = newAvail;
+        _disableInitializers();
     }
 
     /// @notice Initialization funciton for the DeqRouter contract
@@ -73,8 +74,14 @@ contract DeqRouter is PausableUpgradeable, AccessControlDefaultAdminRulesUpgrade
 
     /// @notice Swaps an ERC20 token to staked Avail
     /// @param allowanceTarget Address of the allowance target from 0x API
+    /// @param deadline Deadline for the swap
     /// @param data Data for the swap from 0x API
-    function swapERC20ToStAvail(address allowanceTarget, bytes calldata data) external whenNotPaused {
+    function swapERC20ToStAvail(address allowanceTarget, uint256 deadline, bytes calldata data)
+        external
+        whenNotPaused
+    {
+        // slither-disable-next-line timestamp
+        if (block.timestamp > deadline) revert ExpiredDeadline();
         (IERC20 tokenIn, IERC20 tokenOut, uint256 inAmount, uint256 minOutAmount,) =
             abi.decode(data[4:], (IERC20, IERC20, uint256, uint256, Transformation[]));
         if (address(tokenOut) != address(avail)) revert InvalidOutputToken();
@@ -93,6 +100,10 @@ contract DeqRouter is PausableUpgradeable, AccessControlDefaultAdminRulesUpgrade
     /// @notice Swaps an ERC20 token to staked Avail with permit
     /// @param allowanceTarget Address of the allowance target from 0x API
     /// @param data Data for the swap from 0x API
+    /// @param deadline Deadline for swap and permit execution
+    /// @param v Signature v
+    /// @param r Signature r
+    /// @param s Signature s
     function swapERC20ToStAvailWithPermit(
         address allowanceTarget,
         bytes calldata data,
@@ -101,6 +112,8 @@ contract DeqRouter is PausableUpgradeable, AccessControlDefaultAdminRulesUpgrade
         bytes32 r,
         bytes32 s
     ) external whenNotPaused {
+        // slither-disable-next-line timestamp
+        if (block.timestamp > deadline) revert ExpiredDeadline();
         (IERC20 tokenIn, IERC20 tokenOut, uint256 inAmount, uint256 minOutAmount,) =
             abi.decode(data[4:], (IERC20, IERC20, uint256, uint256, Transformation[]));
         if (address(tokenOut) != address(avail)) revert InvalidOutputToken();
@@ -119,8 +132,11 @@ contract DeqRouter is PausableUpgradeable, AccessControlDefaultAdminRulesUpgrade
     }
 
     /// @notice Swaps ETH to staked Avail
+    /// @param deadline Deadline for the swap
     /// @param data Data for the swap from 0x API
-    function swapETHtoStAvail(bytes calldata data) external payable whenNotPaused {
+    function swapETHtoStAvail(uint256 deadline, bytes calldata data) external payable whenNotPaused {
+        // slither-disable-next-line timestamp
+        if (block.timestamp > deadline) revert ExpiredDeadline();
         (address tokenIn, IERC20 tokenOut, uint256 inAmount, uint256 minOutAmount,) =
             abi.decode(data[4:], (address, IERC20, uint256, uint256, Transformation[]));
         if (address(tokenIn) != 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE) revert InvalidInputToken();

src/StakedAvail.sol

@@ -48,6 +48,7 @@ contract StakedAvail is
     constructor(IERC20 newAvail) {
         if (address(newAvail) == address(0)) revert ZeroAddress();
         avail = newAvail;
+        _disableInitializers();
     }
 
     /// @notice Initializes the StakedAvail contract with governance, updater, depository, and withdrawal helper

src/interfaces/IAvailDepository.sol

@@ -7,6 +7,8 @@ interface IAvailDepository {
     error OnlyDepositor();
     error ZeroAddress();
 
+    event Deposit(uint256 amount);
+
     function updateDepository(bytes32 _depository) external;
     function deposit() external;
 }

src/interfaces/IDeqRouter.sol

@@ -8,13 +8,14 @@ interface IDeqRouter {
     }
 
     error ExceedsSlippage();
+    error ExpiredDeadline();
     error InvalidInputAmount();
     error InvalidInputToken();
     error InvalidOutputToken();
     error SwapFailed(string msg);
     error ZeroAddress();
 
-    function swapERC20ToStAvail(address allowanceTarget, bytes calldata data) external;
+    function swapERC20ToStAvail(address allowanceTarget, uint256 deadline, bytes calldata data) external;
 
     function swapERC20ToStAvailWithPermit(
         address allowanceTarget,
@@ -25,5 +26,5 @@ interface IDeqRouter {
         bytes32 s
     ) external;
 
-    function swapETHtoStAvail(bytes calldata data) external payable;
+    function swapETHtoStAvail(uint256 deadline, bytes calldata data) external payable;
 }

test/AvailDepositoryTest.t.sol

@@ -63,7 +63,7 @@ contract AvailDepositoryTest is Test {
         assertEq(address(newDepository.avail()), rand);
     }
 
-    function testRevert_Initialize(
+    function testRevertZeroAddress_Initialize(
         address rand,
         address newGovernance,
         address newPauser,
@@ -77,7 +77,13 @@ contract AvailDepositoryTest is Test {
                         || newAvailDepositoryAddr == bytes32(0)
                 )
         );
-        AvailDepository newDepository = new AvailDepository(IERC20(rand), IAvailBridge(rand));
+        AvailDepository newDepository = AvailDepository(
+            address(
+                new TransparentUpgradeableProxy(
+                    address(new AvailDepository(IERC20(rand), IAvailBridge(rand))), makeAddr("rand"), ""
+                )
+            )
+        );
         assertEq(address(newDepository.avail()), rand);
         vm.expectRevert(IAvailDepository.ZeroAddress.selector);
         newDepository.initialize(newGovernance, newPauser, newDepositor, newAvailDepositoryAddr);
@@ -146,4 +152,22 @@ contract AvailDepositoryTest is Test {
         // bridge burns the deposited amount
         assertEq(avail.balanceOf(address(stakedAvail)), 0);
     }
+
+    function testRevertOnlyOwner_withdraw(IERC20 token, uint256 amount) external {
+        address from = makeAddr("from");
+        vm.assume(from != owner);
+        vm.expectRevert(
+            abi.encodeWithSelector(IAccessControl.AccessControlUnauthorizedAccount.selector, from, bytes32(0))
+        );
+        vm.prank(from);
+        depository.withdraw(token, amount);
+    }
+
+    function test_withdraw(uint256 amount) external {
+        IERC20 token = new MockERC20("Token", "TKN");
+        deal(address(token), address(depository), amount);
+        vm.prank(owner);
+        depository.withdraw(token, amount);
+        assertEq(token.balanceOf(owner), amount);
+    }
 }

test/AvailWithdrawalHelperTest.t.sol

@@ -52,15 +52,19 @@ contract AvailWithdrawalHelperTest is Test {
         assertEq(address(newWithdrawalHelper.avail()), rand);
     }
 
-    function testRevert_initialize(
+    function testRevertZeroAddress_initialize(
         address rand,
         address newGovernance,
         address newPauser,
         address newStakedAvail,
         uint256 amount
     ) external {
         vm.assume(rand != address(0));
-        AvailWithdrawalHelper newWithdrawalHelper = new AvailWithdrawalHelper(IERC20(rand));
+        AvailWithdrawalHelper newWithdrawalHelper = AvailWithdrawalHelper(
+            address(
+                new TransparentUpgradeableProxy(address(new AvailWithdrawalHelper(IERC20(rand))), makeAddr("rand"), "")
+            )
+        );
         assertEq(address(newWithdrawalHelper.avail()), rand);
         vm.assume(newGovernance == address(0) || newPauser == address(0) || newStakedAvail == address(0));
         vm.expectRevert(IAvailWithdrawalHelper.ZeroAddress.selector);