[VEN-2240]: Certik audit fixes on token bridge

contracts/Bridge/BaseXVSProxyOFT.sol

@@ -21,8 +21,8 @@
 
 abstract contract BaseXVSProxyOFT is Pausable, ExponentialNoError, BaseOFTV2 {
     using SafeERC20 for IERC20;
     IERC20 internal immutable innerToken;
     uint256 internal immutable ld2sdRate;
 
     /**
      * @notice The address of ResilientOracle contract wrapped in its interface.
@@ -61,12 +61,12 @@
      */
     mapping(uint16 => uint256) public chainIdToLast24HourReceiveWindowStart;
     /**
-     * @notice Address on which cap check and bound limit is not appicable.
+     * @notice Address on which cap check and bound limit is not applicable.
      */
     mapping(address => bool) public whitelist;
 
     /**
-     * @notice Emmited when address is added to whitelist.
+     * @notice Emitted when address is added to whitelist.
      */
     event SetWhitelist(address indexed addr, bool isWhitelist);
     /**
@@ -100,7 +100,7 @@
 
     /**
      * @param tokenAddress_ Address of the inner token.
-     * @param sharedDecimals_ No of shared decimals.
+     * @param sharedDecimals_ Number of shared decimals.
      * @param lzEndpoint_ Address of the layer zero endpoint contract.
      * @param oracle_ Address of the price oracle.
      * @custom:error ZeroAddressNotAllowed is thrown when token contract address is zero.
@@ -115,18 +115,17 @@
     ) BaseOFTV2(sharedDecimals_, lzEndpoint_) {
         ensureNonzeroAddress(tokenAddress_);
         ensureNonzeroAddress(lzEndpoint_);
+        ensureNonzeroAddress(oracle_);
 
         innerToken = IERC20(tokenAddress_);
 
         (bool success, bytes memory data) = tokenAddress_.staticcall(abi.encodeWithSignature("decimals()"));
         require(success, "ProxyOFT: failed to get token decimals");
         uint8 decimals = abi.decode(data, (uint8));
 
         require(sharedDecimals_ <= decimals, "ProxyOFT: sharedDecimals must be <= decimals");
         ld2sdRate = 10 ** (decimals - sharedDecimals_);
 
-        ensureNonzeroAddress(oracle_);
-
         emit InnerTokenAdded(tokenAddress_);
         emit OracleChanged(address(0), oracle_);
 
@@ -154,7 +153,7 @@
      * @custom:event Emits SetMaxSingleTransactionLimit with old and new limit associated with chain id.
      */
     function setMaxSingleTransactionLimit(uint16 chainId_, uint256 limit_) external onlyOwner {
         require(limit_ <= chainIdToMaxDailyLimit[chainId_], "Single transaction limit > Daily limit");
         emit SetMaxSingleTransactionLimit(chainId_, chainIdToMaxSingleTransactionLimit[chainId_], limit_);
         chainIdToMaxSingleTransactionLimit[chainId_] = limit_;
     }
@@ -167,7 +166,7 @@
      * @custom:event Emits setMaxDailyLimit with old and new limit associated with chain id.
      */
     function setMaxDailyLimit(uint16 chainId_, uint256 limit_) external onlyOwner {
         require(limit_ >= chainIdToMaxSingleTransactionLimit[chainId_], "Daily limit < single transaction limit");
         emit SetMaxDailyLimit(chainId_, chainIdToMaxDailyLimit[chainId_], limit_);
         chainIdToMaxDailyLimit[chainId_] = limit_;
     }
@@ -180,7 +179,7 @@
      * @custom:event Emits setMaxSingleReceiveTransactionLimit with old and new limit associated with chain id.
      */
     function setMaxSingleReceiveTransactionLimit(uint16 chainId_, uint256 limit_) external onlyOwner {
         require(limit_ <= chainIdToMaxDailyReceiveLimit[chainId_], "single receive transaction limit > Daily limit");
         emit SetMaxSingleReceiveTransactionLimit(chainId_, chainIdToMaxSingleReceiveTransactionLimit[chainId_], limit_);
         chainIdToMaxSingleReceiveTransactionLimit[chainId_] = limit_;
     }
@@ -193,7 +192,7 @@
      * @custom:event Emits setMaxDailyReceiveLimit with old and new limit associated with chain id.
      */
     function setMaxDailyReceiveLimit(uint16 chainId_, uint256 limit_) external onlyOwner {
         require(
             limit_ >= chainIdToMaxSingleReceiveTransactionLimit[chainId_],
             "Daily limit < single receive transaction limit"
         );
@@ -203,7 +202,7 @@
 
     /**
      * @notice Sets the whitelist address to skip checks on transaction limit.
-     * @param user_ Adress to be add in whitelist.
+     * @param user_ Address to be add in whitelist.
      * @param val_ Boolean to be set (true for user_ address is whitelisted).
      * @custom:access Only owner.
      * @custom:event Emits setWhitelist.
@@ -238,10 +237,27 @@
         emit TrustedRemoteRemoved(remoteChainId_);
     }
 
+    function retryMessage(
+        uint16 _srcChainId,
+        bytes calldata _srcAddress,
+        uint64 _nonce,
+        bytes calldata _payload
+    ) public payable override {
+        bytes memory trustedRemote = trustedRemoteLookup[_srcChainId];
+        // it will still block the message pathway from (srcChainId, srcAddress). should not receive message from untrusted remote.
+        require(
+            _srcAddress.length == trustedRemote.length &&
+                trustedRemote.length > 0 &&
+                keccak256(_srcAddress) == keccak256(trustedRemote),
+            "LzApp: invalid source sending contract"
+        );
+        super.retryMessage(_srcChainId, _srcAddress, _nonce, _payload);
+    }
+
     /**
      * @notice Empty implementation of renounce ownership to avoid any mishappening.
      */
     function renounceOwnership() public override {}
 
     /**
      * @notice Return's the address of the inner token of this bridge.
@@ -341,23 +357,6 @@
         return innerToken.balanceOf(to_) - before;
     }
 
-    function _nonblockingLzReceive(
-        uint16 _srcChainId,
-        bytes memory _srcAddress,
-        uint64 _nonce,
-        bytes memory _payload
-    ) internal override {
-        bytes memory trustedRemote = trustedRemoteLookup[_srcChainId];
-        // if will still block the message pathway from (srcChainId, srcAddress). should not receive message from untrusted remote.
-        require(
-            _srcAddress.length == trustedRemote.length &&
-                trustedRemote.length > 0 &&
-                keccak256(_srcAddress) == keccak256(trustedRemote),
-            "LzApp: invalid source sending contract"
-        );
-        super._nonblockingLzReceive(_srcChainId, _srcAddress, _nonce, _payload);
-    }
-
     function _ld2sdRate() internal view override returns (uint256) {
         return ld2sdRate;
     }

contracts/Bridge/XVSBridgeAdmin.sol

@@ -33,10 +33,8 @@ contract XVSBridgeAdmin is AccessControlledV8 {
 
     /**
      * @param accessControlManager_ Address of access control manager contract.
-     * @custom:error ZeroAddressNotAllowed is thrown when accessControlManager contract address is zero.
      */
     function initialize(address accessControlManager_) external initializer {
-        ensureNonzeroAddress(accessControlManager_);
         __AccessControlled_init(accessControlManager_);
     }
 
@@ -66,14 +64,14 @@ contract XVSBridgeAdmin is AccessControlledV8 {
     }
 
     /**
-     * @notice A registry of functions that are allowed to be executed from proposals
+     * @notice A setter for the registry of functions that are allowed to be executed from proposals.
      * @param signatures_  Function signature to be added or removed.
      * @param active_ bool value, should be true to add function.
      */
     function upsertSignature(string[] calldata signatures_, bool[] calldata active_) external onlyOwner {
         uint256 signatureLength = signatures_.length;
         require(signatureLength == active_.length, "Input arrays must have the same length");
-        for (uint256 i; i < signatureLength; i++) {
+        for (uint256 i; i < signatureLength; ) {
             bytes4 sigHash = bytes4(keccak256(bytes(signatures_[i])));
             bytes memory signature = bytes(functionRegistry[sigHash]);
             if (active_[i] && signature.length == 0) {
@@ -83,17 +81,19 @@ contract XVSBridgeAdmin is AccessControlledV8 {
                 delete functionRegistry[sigHash];
                 emit FunctionRegistryChanged(signatures_[i], false);
             }
+            unchecked {
+                i++;
+            }
         }
     }
 
     /**
-     * @notice This function transfer the ownership of the bridge from this contract to new owner.
+     * @notice This function transfers the ownership of the bridge from this contract to new owner.
      * @param newOwner_ New owner of the XVS Bridge.
      * @custom:access Controlled by AccessControlManager.
      */
     function transferBridgeOwnership(address newOwner_) external {
         _checkAccessAllowed("transferBridgeOwnership(address)");
-        ensureNonzeroAddress(newOwner_);
         XVSBridge.transferOwnership(newOwner_);
     }
 

contracts/Bridge/XVSProxyOFTDest.sol

@@ -14,7 +14,7 @@ import { BaseXVSProxyOFT } from "./BaseXVSProxyOFT.sol";
 
 contract XVSProxyOFTDest is BaseXVSProxyOFT {
     /**
-     * @notice Emits when stored message dropped without successfull retrying.
+     * @notice Emits when stored message dropped without successful retrying.
      */
     event DropFailedMessage(uint16 srcChainId, bytes indexed srcAddress, uint64 nonce);
 

contracts/Bridge/XVSProxyOFTSrc.sol

@@ -16,7 +16,7 @@ import { BaseXVSProxyOFT } from "./BaseXVSProxyOFT.sol";
 contract XVSProxyOFTSrc is BaseXVSProxyOFT {
     using SafeERC20 for IERC20;
     /**
-     * @notice total amount is transferred from this chain to other chains.
+     * @notice Total amount that is transferred from this chain to other chains.
      */
     uint256 public outboundAmount;
 
@@ -25,7 +25,7 @@ contract XVSProxyOFTSrc is BaseXVSProxyOFT {
      */
     event FallbackWithdraw(address indexed to, uint256 amount);
     /**
-     * @notice Emits when stored message dropped without successfull retrying.
+     * @notice Emits when stored message dropped without successful retrying.
      */
     event DropFailedMessage(uint16 srcChainId, bytes indexed srcAddress, uint64 nonce);
 
@@ -42,7 +42,10 @@ contract XVSProxyOFTSrc is BaseXVSProxyOFT {
      * @param amount_ The amount of withdrawal
      */
     function fallbackWithdraw(address to_, uint256 amount_) external onlyOwner {
-        outboundAmount -= amount_;
+        require(outboundAmount >= amount_, "Withdraw amount should be less than outbound amount");
+        unchecked {
+            outboundAmount -= amount_;
+        }
         _transferFrom(address(this), to_, amount_);
         emit FallbackWithdraw(to_, amount_);
     }

contracts/Bridge/token/TokenController.sol

@@ -21,13 +21,13 @@ contract TokenController is Ownable, Pausable {
     /**
      * @notice A Mapping used to keep track of the blacklist status of addresses.
      */
-    mapping(address => bool) public _blacklist;
+    mapping(address => bool) internal _blacklist;
     /**
      * @notice A mapping is used to keep track of the maximum amount a minter is permitted to mint.
      */
     mapping(address => uint256) public minterToCap;
     /**
-     * @notice A Mapping used to keep track of the amount i.e already m inted by minter.
+     * @notice A Mapping used to keep track of the amount i.e already minted by minter.
      */
     mapping(address => uint256) public minterToMintedAmount;
 
@@ -106,14 +106,15 @@ contract TokenController is Ownable, Pausable {
     }
 
     /**
-     * @notice Sets the minitng cap for minter.
+     * @notice Sets the minting cap for minter.
      * @param minter_ Minter address.
      * @param amount_ Cap for the minter.
      * @custom:access Controlled by AccessControlManager.
      * @custom:event Emits MintCapChanged.
      */
     function setMintCap(address minter_, uint256 amount_) external {
         _ensureAllowed("setMintCap(address,uint256)");
+        require(amount_ > minterToMintedAmount[minter_], "New cap should be greater than minted tokens");
         minterToCap[minter_] = amount_;
         emit MintCapChanged(minter_, amount_);
     }
@@ -159,12 +160,15 @@ contract TokenController is Ownable, Pausable {
             revert MintLimitExceed();
         }
         minterToMintedAmount[from_] = totalMintedNew;
-        uint256 availableLimit = mintingCap - totalMintedNew;
+        uint256 availableLimit;
+        unchecked {
+            availableLimit = mintingCap - totalMintedNew;
+        }
         emit MintLimitDecreased(from_, availableLimit);
     }
 
     /**
-     * @dev This is post hook of burn function, increases minitng limit of the minter.
+     * @dev This is post hook of burn function, increases minting limit of the minter.
      * @param from_ Minter address.
      * @param amount_  Amount burned.
      */

contracts/Bridge/token/XVS.sol

@@ -18,12 +18,12 @@ contract XVS is ERC20, TokenController {
     /**
      * @notice Creates `amount_` tokens and assigns them to `account_`, increasing
      * the total supply. Checks access and eligibility.
-     * @param account_ Address to which tokens be assigned.
+     * @param account_ Address to which tokens are assigned.
      * @param amount_ Amount of tokens to be assigned.
      * @custom:access Controlled by AccessControlManager.
      * @custom:event Emits MintLimitDecreased with new available limit.
-     * @custom:error MintNotAllowed is thrown when minting is not allowed to from_ address.
-     * @custom:error MintLimitExceed is thrown when minting amount exceed the maximum cap.
+     * @custom:error MintNotAllowed is thrown when minting is not allowed to to_ address.
+     * @custom:error MintLimitExceed is thrown when minting amount exceeds the maximum cap.
      */
     function mint(address account_, uint256 amount_) external whenNotPaused {
         _ensureAllowed("mint(address,uint256)");

test/proxyOFT.ts

@@ -508,6 +508,33 @@ describe("Proxy OFTV2: ", function () {
     accessControlManager.isAllowedToCall.returns(true);
   });
 
+  it("Reverts if try to set cap less than already minted tokens", async function () {
+    const initialAmount = ethers.utils.parseEther("20", 18);
+    await localToken.connect(acc2).faucet(initialAmount);
+    await localToken.connect(acc2).approve(localOFT.address, initialAmount);
+    const amount = ethers.utils.parseEther("10", 18);
+    const acc3AddressBytes32 = ethers.utils.defaultAbiCoder.encode(["address"], [acc3.address]);
+    const nativeFee = (
+      await localOFT.estimateSendFee(remoteChainId, acc3AddressBytes32, amount, false, defaultAdapterParams)
+    ).nativeFee;
+
+    await localOFT
+      .connect(acc2)
+      .sendFrom(
+        acc2.address,
+        remoteChainId,
+        acc3AddressBytes32,
+        amount,
+        [acc2.address, ethers.constants.AddressZero, defaultAdapterParams],
+        { value: nativeFee },
+      ),
+      // Msg should reach remote chain
+      expect(await remoteEndpoint.inboundNonce(localChainId, localPath)).equals(1);
+    await expect(remoteToken.connect(acc1).setMintCap(remoteOFT.address, convertToUnit(1, 18))).to.be.revertedWith(
+      "New cap should be greater than minted tokens",
+    );
+  });
+
   it("Reverts on remote chain if minting cap is reached", async function () {
     await remoteToken.connect(acc1).setMintCap(remoteOFT.address, convertToUnit(10, 18));
     expect(await remoteEndpoint.inboundNonce(localChainId, localPath)).lessThanOrEqual(0);