tags: Final Report

TGT Finance Audit

Copyright © 2022 by Verilog Solutions. All rights reserved.
February 24, 2022
by Verilog Solutions

TGT Cover

This report presents Verilog’s smart contract auditing engagement with TGT Protocol. TGT Protocol is one of the first lending protocols and margin trading platforms on the Emerald Paratime of Oasis Network.

Table of Content

Project Summary

TGT Finance is a lending protocol built on Oasis Network. TGT Finance utilized a pooled collateral/deposit model to increase capital efficiency. TGT Finance also provides native leverage trading products, which offer a streamlined user experience to traders.

Service Scope

Our audit is conducted on the main branch, with commit hash 1c66a650cd01d24bf2c27c2e49e85b1419b004d2.

Our engagement with TGT Protocol includes the following two services:

  1. Pre-Audit Consulting Service

    As a part of the pre-audit service, the Verilog team worked closely with the TGT development team to discuss potential vulnerability and smart contract development best practices in a timely fashion. Verilog team is very appreciative of establishing an efficient and effective communication channel with the TGT team, as new findings were often exchanged promptly and fixes were deployed quickly, during the preliminary report stage.

  2. Audit Service

    The Verilog team conducted a thorough study of the TGT code, with the TGT architecture graph and UML graph presented below in the TGT Architecture section. The list of findings, along with the severity and solution, is available under section Findings & Improvement Suggestions.

Architecture

TGT-Architecture

Listed below are the major smart contracts in the TGT Finance Protocol.

Privileged Roles

  1. FToken.sol

    • controller:
      • _reduceReserves()
      • _addReservesFresh()
    • component: it can be controller, this contract itself or eligible FToken.sol contracts.
      • transferToUser()
      • transferIn()
      • addTotalCash()
      • subTotalCash()
    • vault: permited vaults
      • repayInternalForLeverage()
      • addReservesForLeverage()

  2. TokenRewardPool.sol

    • owner
      • createPool()
      • setPoolRewardsBlockCount()
      • setPoolRewardsDistributor()

  3. BandPriceOracle.sol

    • owner
      • setTokenName()
      • setBaseToken()

  4. ChainLinkPriceOracle.sol

    • owner
      • setBaseToken()
      • setBufferDay()
      • setPriceFeed()

  5. BankController.sol

    • multisig
      • proposeNewAdmin()
      • reduceReserves()
      • batchReduceReserves()
      • batchReduceAllReserves(address[], address payable)
      • batchReduceAllReserves(address payable)
    • admin:
      • setTransferEthGasCost()
      • setOracle()
      • supportMarket()
      • supportVault()
      • _setCollateralAbility()
      • setCloseFactor()
      • setMarketIsValid()
      • setPauser()
    • pauser:
      • setPaused()
      • setUnpaused()

  6. WorkerConfig.sol

    • owner:
      • setOracle()
      • setConfigs()

  7. WNativeReplayer.sol

    • owner:
      • setCallerOk(): set and remove whitelisted caller
    • whitelistedCallers():
      • withdraw()

  8. UniswapWorker.sol

    • owner:
      • setParams(): set two kinds strategists.
      • setStrategyOk(): set or remove strategists.

  9. InterestRateModel.sol

    • owner
      • setSlope1AndSlope2()

  10. TokenVault.sol

    • owner:
      • grant or revoke permissions
    • worker:
      • withdraw()

Findings & Improvement Suggestions

InformationalMinorMediumMajorCritical

Total Acknowledged Resolved
Critical 0 0 0
Major 3 3 3
Medium 3 3 3
Minor 11 11 11
Informational 16 16 16

Major

  1. Reentrancy attack risk at FToken.deposit() (FToken.deposit(): L296). Major
    Description: Possible reentrancy attack might happen at FToken.deposit(). It associates with sending native token back to account which might have reentrancy attack inside its fallback functions.
    Recommendation: Add nonReenntrant modifier for FToken.deposit() and remove the nonReentrant for mint().
    Result: Fixed in commit 135479ea1fd773e1446b07451dfa399e03071be2.

  2. Change the constructor to a initialize function if it’s an updagrable contract (WNativeRelayer.sol, TokenVault.sol).Major
    Description: Avoid using constructor or assign variables values inside constructor when writing an upgradable contract.
    Recommendation: The value will not be written to proxy contract’s storage if assigning variables values inside constructor.
    Result: Fixed in commit 6fe1904d5dca8e5b0cadf2d5c0db22d5162c31d7.

  3. Logical conflict. pending owner cannot claim ownership (TokenVault.sol). Major
    Description: pending owner cannot claim pending ownership since transferOwnership function can only be called by owner.
    Recommendation: Rewrite the function claimOwnership.
    Result: Fixed in commit fa6df5b1f130a5182b25eb9cbacc0f5a55232dd7.

Medium

  1. Returns unexpected value when utilization is bigger or equal to MAX_USAGE_RATE at InterestRateModel._getPureAPR() based on comment (InterestRateModel.sol: L75).Medium
    Description: The return value should be BASIC_INTEREST + interestSlope1 + interestSlope2 when utilization is bigger or equal to MAX_USAGE_RATE.
    Recommendation:

    



        else {
        // Not possible, but just in case - 130% APY
        return BASIC_INTEREST + interestSlope1 + interestSlope2;
    }

    Result: Fixed in commit ab2fb8ed5fe8529892ae72f318cc71c807e3f244.

  2. OwnableUpgradeSafe andReentrancyGuardUpgradeSafe are not initialized (TokenVault.sol: L147, L169). Medium
    Description:OwnableUpgradeSafe in Whitelist and ReentrancyGuardUpgradeSafe in TokenVault contract are not initialized.
    Recommendation: Initialize OwnableUpgradeSafe ReentrancyGuardUpgradeSafe inside the initialize function.
    Result: Fixed in commit fa6df5b1f130a5182b25eb9cbacc0f5a55232dd7.

  3. Reentrancy attack risk at Vault.work() and Vault.kill() (Vault.work(): L197, Vault.kill(): L309). Medium
    Description: Users interact with vault to perform margin trading through functions Vault.work() and Vault.kill(). There are many calls to external contracts and complicated variable calculations inside these two functions. nonReentrant modifier is recommended to avoid possible reentrancy attack.
    Recommendation: Add nonReentrant modifier to Vault.work() and Vault.kill().
    Result: Fixed in commit fb4b983c5bc78b9dd5055274213205a74eabf0c0.

Minor

  1. Unused OwnableUpgradeSafe and library SafeMath in (StrategySaveBaseTokenOnly.sol, StrategyWithdrawTrading.sol, WorkerConfig.sol) Minor
    Description: There is no function that requires the owner role so OwnableUpgradeSafe is unnecessary. Library SafeMath is currently unused.
    Recommendation: Remove unused inheritance and libraries.
    Result: Fixed in commit 43dfbb927e4f99c58fa5e5e745adc4d716407e26.

  2. Unsued modifier. (UniswapWorker.sol: L87, Vault.sol: L103) Minor
    Description: modifier onlyEOA in UniswapWorker.sol and inExec in Vault.sol are unused.
    Recommendation: Remove the unused modifiers.
    Result: Fixed in commit a39151f24de112244b0051de0aaacd71fb4fb091.

  3. Variable declarations shadow existing declarations (BandPriceOracle.sol:L34, L44 FToken.sol:L394, L403) Minor
    Description:

    • Variable declearation uint256 totalCash in calcExchangeRate() (L394) and in exchangeRateAfter() shadows exisiting declearation.
    • The return value price of BandPriceOracle.getPrice()and the variable price defined in BandPriceOracle.get() shadow the existing valuable price.

    Recommendation: Rename those variables.
    Result: Fixed in commit a39151f24de112244b0051de0aaacd71fb4fb091.

  4. The variable address worker in event Work() not indexed (Vault.sol: L30). Minor
    Description: The variable address worker in event Work() not indexed.
    Recommendation: Add indexed modifier to the variable address
    Result: Fixed in commit 1c66a650cd01d24bf2c27c2e49e85b1419b004d2.

  5. Unused function parameter address _underlying in transferToUserInternal() (FToken.()L157). Minor
    Description: Unused function parameter address _underlying.
    Recommendation: Remove or Comment out the variable name. Otherwise, put it in a placeholder expression _underlying;
    Result:Fixed in commit 3a0b6664ef947ddaeb2f4d6474c18068c1cbdce3.

  6. Scope and modifier for FToken.transferIn() function(FToken.transferIn(): L172, FToken.approve(): L255). minor
    Description: FToken.transferIn() is currently only used as external functions here. To enhance security, we have two alternative modification suggestions for this function.
    Recommendation:

    • Add a nonReentrant modifier to this function and make it an external function.
    • Make this function an internal or private function

    Result: Fixed in commit 9fba8673ceb0c8c68300846fb2d819aa451750ee.

  7. Function calls might fail if farm contract address is zero address (FToken.transferTokens(): L249, FToken.deposit(): L301, FToken.withdrawInternal(): L507, FToken.seizeInternal(): L886). Minor
    Description: There are calls to farm contract, which is set in the contract ConfigurableInterestVaultConfig. Calls might fail if farm contract is not set or set to zero address for current FToken contract.
    Recommendation: Do Address zero check first on farm address then do interactions with it.

    



     (address farm, uint256 poolId) = config.getFarmConfig(address(this));
 if(farm != address(0)) {
 // do calls to farm contract
 }

    Result: Fixed in commit 70d0c87fff53646e50d4a34e283b84486a6298fc.

  8. event PauserSet() lacks corrsponding newPauser and oldPauser (BankController: L37). Minor
    Description: The PauserSet() decleration does not include corrsponding data fields (newPauser and oldPauser).
    Recommendation: Add newPauser and oldPauser into PauserSet() decleration.
    Result: Fixed in commit f6193adc03ee7c4ff52ef1127de8c2b0c320b7b3.

  9. Redundant type casting from address to IFToken (BankController: L274). Minor
    Description:

    


    function checkAccountsIn(address account, IFToken fToken) external view returns (bool) {
    return markets[IFToken(address(fToken)).underlying()].accountsIn[account];
}

    Recommendation: Replace with the following code.

    


    function checkAccountsIn(address account, IFToken fToken) external view returns (bool) {
    return markets[fToken.underlying()].accountsIn[account];
}

    Result: Fixed in commit 14d4cc7c64cd1e2bfc1caa3ba1d9f8782ec2eb7f.

  10. Sanitize Input Minor
    Description: We suggest adding input sanitization checks when assigning function input parameters to variables.
    Recommendation: Add input sanitization checks when assigning function input parameters to variables. For example:

    • Zero address check for minter and account in BankController.mintCheck() and BankController.borrowCheck()
    • Input number range check for _minDebtSize in ConfigurableInterestVaultConfig.setMinDebtSize()

    Result: Fixed in commit c7ce45de5d308596719a376e97878a0b7ea9cf83.

  11. Check if pool already exists when creating new pool (TokenRewardPool.createPool(): L86) Minor
    Description: Check if pool already exists when creating new pool.
    Recommendation: Add a mapping to track all added pool. Check if pool existed in TokenRewardPool.createPool() first.

    












    // add a mapping to track added pool
mapping(address => mapping(address => bool)) isPoolCreated;

function createPool(
    IERC20Upgradeable _stakingToken,
    IERC20Upgradeable _rewardsToken,
    address _rewardsDistributor
) public onlyOwner {
    // check pool
    require(!isPoolCreated[_stakingToken][_rewardsToken], "TRP: POOL EXISTED!");
    // ...
    
    isPoolCreated[_stakingToken][_rewardsToken] = true;

    Result: Fixed in commit 85cfad57f495e8a138ab7f92995be6e14b7a16b5.

Informational

  1. General practice suggestions. Informational
    Description: The are many functions associated with transferring users’ funds following a series of other operations (i.e. FToken.repay()). We recommend transferring in funds first then do these operations.
    Recommendation: Transfer in funds first then do the rest of operations.
    Result: Fixed in commit a39151f24de112244b0051de0aaacd71fb4fb091. File deleted.

  2. The IDebtToken interface is defined but never used (IDebtToken). Informational
    Description: The IDebtToken interface is defined but not implemented by any contracts.
    Recommendation: Remove the interface.
    Result: Fixed in commit ed1298dff74e9636948932aa2a58b843470d92f2. File deleted.

  3. BankController.sol Should implement IBankController interface (BankController: L16). Informational
    Description: Not implementing the interface may cause the run time error if function is defined in BankController.sol without a declaration in IBankController.
    Recommendation: Add IBankController in the list of implementation.
    Result: Fixed in commit 69d3c5a6635fdf2c3f364684e0a2cba6f456b8d4

  4. InterestRateModel should implement the IInterestRateModel interface (InterestRateModel.sol: L7). Informational
    Description: Not implementing the interface may cause the run time error if function is defined in InterestRateModel without a declaration in IInterestRateModel.
    Recommendation: Add IInterestRateModel in the list of implementation.
    Result: Fixed in commit df588cccd2fe8b8a41dd5cc27d1780a8ac5f5001.

  5. Redundant interface IInterestRateModel.sol and InterestModel.sol.
    Description: Two interfaces are about interest rate model.
    Recommendation: Keep IInterestRateModel.sol.
    Result: Fixed in commit 43261b8c350657091507bd837d578f6e95c0aa50.

  6. FToken.sol contract should implement the IERC20 standards (FToken.transfer(): L205, FToken.approve(): L255). Informational
    Description: The FToken does not implement the standard IERC20 interface. FToken.transfer() does not have bool return and FToken.approve() does not emit Approval event. Thus it does not follows standards
    Recommendation: Modifies the functions to follow the standard IERC20 interface and also modifiers the IFToken.sol as well.
    Result: Fixed in commit 8c1ed9f9061216e334c88d02ca6025e379fef636.

  7. Comments // 2. Convert farming tokens to base tokens. is wrong (StrategySaveBaseTokenOnly: L43). Informational
    Description: The comment should be convert base tokens to farm tokens.
    Recommendation: Change comment to Convert base tokens to farm tokens.
    Result: Fixed in commit ab2fb8ed5fe8529892ae72f318cc71c807e3f244.

  8. Add token balance differences check (StrategySaveBaseTokenOnly.executeWithData(): L24, StrategyWithdrawTrading.executeWithData(): L23). Informational
    Description: function executeWithData convert all base token to farm token. A balance difference check for farm token could be added to make sure farm token balance increases after swap.
    Recommendation:
    function StrategySaveBaseTokenOnly.executeWithData() convert all base tokens to farm tokens. StrategyWithdrawTrading.executeWithData() convert partial farm tokens / base tokens. Record the tokens balance before and after swap and require that balance difference is greater or equal to amountOutMin.
    Result: Fixed in commit a39151f24de112244b0051de0aaacd71fb4fb091. Required after balance bigger than before balance.

  9. Remove payable keyword in function executeWithData (StrategySaveBaseTokenOnly.executeWithData(): L30, IStrategy.executeWithData(): L5). Informational
    Description: All IStrategy contracts we currently have only deal with ERC20 tokens. No need for payable keyword.
    Recommendation: Remove payable.
    Result: Fixed in commit a39151f24de112244b0051de0aaacd71fb4fb091.

  10. Use existing onlyAdmin() modifier instead of require(msg.sender==admin) (BankController.sol: L151, L172). Informational
    Description: The _setMarketBorrowSupplyCaps() and setTokenConfig() implment existing authentication login defined in onlyAdmin().
    Recommendation: Replace require(msg.sender==admin) with onlyAdmin() modifier.
    Result: Fixed in commit a39151f24de112244b0051de0aaacd71fb4fb091.

  11. Suggest only receive from wnative (WNativeRelayer.receive(): L37). Informational
    Description: Add a require to receive() to require the msg.sender is wnative. So only wnative can send native tokens to this contract.
    Recommendation:

    


    receive() external payable {
    require(msg.sender == wnative, "WNativeRelayer::onlyWnative");
}

    Result: Fixed in commit 8c9ac7d8d7101da3059df5c1e3c41182af94d710.

  12. OPTICAL_USAGE_RATE, MAX_USAGE_RATE and BASIC_INTEREST can be constant variables (InterestRateModel.sol). Informational
    Description: OPTICAL_USAGE_RATE, MAX_USAGE_RATE and BASIC_INTEREST can be constant variables.
    Recommendation: Set OPTICAL_USAGE_RATE, MAX_USAGE_RATE and BASIC_INTEREST to constant variables.
    Result: Fixed in commit ca81ff479fd0ea435151f4115790eeda0c58b13b.

  13. Wrong error message in withdraw() (TokenVault: L212). Informational
    Description: The error message of require(_receiver != address(0), "Illegal amount" ); unmatch.
    Recommendation: Replace it with "Receiver address is zero".
    Result: Fixed in commit c2e7ca29f122e24624e0aea8725aedd6e3833d80.

  14. Redundant check (TokenVault.withdraw(): L213)Informational
    Description: The check for SToken balance is redundant. safeTransfer will revert on insufficient balance.
    Recommendation: Remove the redundant the require.
    Result: Fixed in commit c4a6b8d55aa0b70ae3e69611e2fe0c67a2393e8b.

  15. Use openzeppelin Address library for onlyEOA modifier (Vault.sol: L85). Informational
    Description: Currently, it only checks if an account is EOA by comparing msg.sender and tx.origin. There are many ways to bypass this check (i.e. using delegatecall).
    Recommendation: Use openzeppelin Address library for onlyEOA modifier to check if an account is EOA.
    Result: Fixed in commit e8e8afe2f11b73b2ee4c5b765ee833a7a9672af4.

  16. Interface IVault.sol is empty. Informational
    Description: IVault interface can be removed from Vault inheritance since IVault is empty.
    Recommendation: Remove IVault.sol.
    Result: Fixed in commit 14d0251eebb299edea73eeea4eb0473e2234b278.

Disclaimer

Verilog receives compensation from one or more clients for performing the smart contract and auditing analysis contained in these reports. The report created is solely for Clients and published with their consent. As such, the scope of our audit is limited to a review of code, and only the code we note as being within the scope of our audit detailed in this report. It is important to note that the Solidity code itself presents unique and unquantifiable risks since the Solidity language itself remains under current development and is subject to unknown risks and flaws. Our sole goal is to help reduce the attack vectors and the high level of variance associated with utilizing new and consistently changing technologies. Thus, Verilog in no way claims any guarantee of security or functionality of the technology we agree to analyze.

In addition, Verilog reports do not provide any indication of the technologies proprietors, business, business model, or legal compliance. As such, reports do not provide investment advice and should not be used to make decisions about investment or involvement with any particular project. Verilog has the right to distribute the Report through other means, including via Verilog publications and other distributions. Verilog makes the reports available to parties other than the Clients (i.e., “third parties”) – on its website in hopes that it can help the blockchain ecosystem develop technical best practices in this rapidly evolving area of innovation.