Sign Up for a Code Audit Today and Get 3 Months of Skynet for Free

When transferring standard ERC20 deflationary tokens, the input amount may not be equal to the received amount due to the charged transaction fee. 

In this project, the token address `principle` is passed into several contracts. For example, in contract `BondDepository`, the function `deposit()` allows the user to buy the bond with `principle`.

```solidity
    function deposit( 
        uint _amount, 
        uint _maxPrice,
        address _depositor
    ) external returns ( uint ) {
        ...
        IERC20( principle ).safeTransferFrom( msg.sender, address(this), _amount );
        IERC20( principle ).approve( address( treasury ), _amount );
        ITreasury( treasury ).deposit( _amount, principle, profit );

        ...
    }
```
If the `principle` is a deflationary token, then the amount `treasury` gets will not match with the amount `BondDepository` received. 

Incompatibility With Deflationary Tokens

Literals with many digits are difficult to read and review.

Too Many Digits

In the contract `Ownable`, the role `_owner` has authority over the following functions:
- `Ownable.renounceManagement()` to  renounce the `_owner`;
- `Ownable.pushManagement(address newOwner_ )` to decide the new owner, but not yet change ownership.

In the contract `RedeemHelper`, the role `_owner` has authority over the following functions:
- `RedeemHelper.addBondContract(address _bond)` to add a bond contract depositors can redeem for;
- `RedeemHelper.removeBondContract(uint _index)` to remove a bond contract depositors can redeem for.

Any compromise to the `_owner` or `_newOwner` account may allow a hacker to take advantage of this authority and disrupt how users interact with bonds. For example, if an attacker acquires ownership privileges, they are able to put a malicious address as a bond address or remove bond addresses to prevent users from redeeming them.

Centralization Related Risks in `RedeemHelper`

In lines 299 - 303 of function `BondingCalculator.markdown()`, the logic indicates that if the `token0` is not TEM, then `token1` must be TEM. This assumes that one of the two tokens in the pair is TEM. If a pair of tokens does not contain TEM, it will return unexpected value. 

`token1` May Not Be TEM 

In the contract `Ownable`, the role `policy` has authority over the following functions:
- `Ownable.renouncePolicy()` to renounce the ownership of the contract;
- `Ownable.pushPolicy( address newPolicy_ )` to transfer ownership to a new owner with the `pull over push` model.

In contract `StakingDistributor`, the role `policy` has authority over the following functions:
- `StakingDistributor.addRecipient()` to add a recipient for token distribution;
- `StakingDistributor.removeRecipient()` to remove a recipient for token distribution;
- `StakingDistributor.setAdjustment()` to modify the control variable parameters through the `Adjust` structure.

Any compromise to these account may allow a hacker to take advantage of this authority and manipulate the project. 

Centralization Related Risks in `StakingDistributor`

The function `Distributor.nextRewardFor()` retrieves the reward of a particular recipient. If duplicate recipients are allowed to be added to `info` via `Distributor.addRecipient()`:
```solidity=463
    function addRecipient( address _recipient, uint _rewardRate ) external onlyPolicy() {
        require( _recipient != address(0) );
        info.push( Info({
            recipient: _recipient,
            rate: _rewardRate
        }));
    }
```
then it is recommended to pass the index as well to `Distributor.nextRewardFor()`. Otherwise, it will always return the first recipient with same address in the `info` structure. 

Duplicate Recipients

In the contract `Ownable`, the role `owner` has authority over the following functions:
- `Ownable.renounceManagement()` to renounce the ownership of the contract;
- `Ownable.pushManagement( address newOwner_ )` to transfer ownership to a new owner with the `pull over push` model.

In contract `Staking`, the role `owner` has authority over the following functions:
- `Staking.setContract()` to modify the `Distributor`, `Warmup` and `Locker` contracts;
- `Staking.setWarmup()` to modify the `Warmup` period.

The role `locker` has authority over the following functions:
- `Staking.giveLockBonus( uint _amount )` to lock amount of SWORD as bonus;
- `Staking.returnLockBonus( uint _amount )` to return amount of SWORD to the contract.
  
Any compromise to these account may allow a hacker to take advantage of this authority and manipulate the project. 

Centralization Related Risks in `Staking`

The function `Staking.claim()` does not have any access control so that anyone can call it to retrieve SWORD from warmup to any recipient and delete the warmup info of the recipient. This might cause unexpected results.

For example, User A locks his deposit by calling `Staking.toggleDepositLock()` so no more deposits can be made for his account. However, User B is able to delete User A's `warmupInfo` by calling `Staking.claim()` with User A's account as the recipient. Then User B can deposit for User A's account again by calling `staking.stake()` with User A's account as the recipient. In this way, the check for `warmupInfo.lock` can be bypassed.

Lack of Access Control on `claim()`

In the contract `Ownable`, the role `manager` has authority over the following functions:
- `Ownable.renounceManagement()` to renounce the ownership of the contract;
- `Ownable.pushManagement( address newOwner_ )` to transfer ownership to a new owner with the `pull over push` model.

In the contract `SwordToken`, the role `manager` has authority over the following functions:
- `SwordToken.setIndex( uint _INDEX )` to set a new index.

Any compromise to the `manager` account may allow a hacker to take advantage of this authority and manipulate the project.

Centralization Related Risks in `SwordToken`

The `transfer()` function computes the `gonValue` variable as follow :
```solidity=1139
uint256 gonValue = value.mul( _gonsPerFragment );
```

However, the `gonsForBalance()` is already designed to compute this value:
```solidity=1123
    function gonsForBalance( uint amount ) public view returns ( uint ) {
        return amount.mul( _gonsPerFragment );
    }
```

Redundant Statements

The function `SwordToken.circulatingSupply()` calculates the total circulating supply of SWORD tokens. In contract `Staking`, the address `warmupContract` will lock the staked SWORD tokens from users until the `warmupPeriod` expires. Additionally, the `locker` locks certain amount of SWORD tokens as bonus. To our understanding, the amount of circulating supply is supposed to exclude the SWORD tokens locked in `warmupContract` and `locker`. 

Possibly Incomplete `circulatingSupply()`

In the contract `Ownable`, the role `owner` has authority over the following functions:
- `renounceManagement()` to renounce the ownership of the contract;
- `pushManagement( address newOwner_ )` to transfer ownership to a new owner with the `pull over push` model.

In contracts `BondDepository` and `BondDepositoryBNB`, the role `owner` has authority over the following functions:
- `initializeBondTerms()` to initialize a `Terms` structure, containing the bond parameters;
- `setBondTerms()` to modify the bond parameters through the `Terms` structure;
- `setAdjustment()` to modify the control variable parameters through the `Adjust` structure;
- `setStaking()` to modify the `Staking` and `StakingHelper` contracts.

Any compromise to these account may allow a hacker to take advantage of this authority and manipulate the project. 

Centralization Related Risks in `BondDepository` and `BondDepositoryBNB`

The function `redeem()` in `BondDepository`/`BondDepositoryBNB` contracts do not have any access control: anyone is able to call it to redeem the bond for an arbitrary recipient. 
```solidity=881
    function redeem( address _recipient, bool _stake ) external returns ( uint ) {        
        Bond memory info = bondInfo[ _recipient ];
        uint percentVested = percentVestedFor( _recipient ); // (blocks since last interaction / vesting term remaining)
      ...
```
First, the `redeem()` function passes the boolean `_stake` to decide whether the redeemed TEM will be staked or sent to the recipient. If a recipient wants to get back the redeemed TEM, and someone else calls the `redeem()` with `_stake = true`, then the TEM will be staked. In this case, the recipient won't be able to get back the TEM immediately. 

Second, the `redeem()` function calls the function `percentVestedFor( _recipient )`, whose return value depends on block number at which it is invoked.
```solidity=1074
    function percentVestedFor( address _depositor ) public view returns ( uint percentVested_ ) {
        Bond memory bond = bondInfo[ _depositor ];
        uint blocksSinceLast = block.number.sub( bond.lastBlock );
        uint vesting = bond.vesting;

        if ( vesting > 0 ) {
            percentVested_ = blocksSinceLast.mul( 10000 ).div( vesting );
        } else {
            percentVested_ = 0;
        }
    }
```

In this case, a recipient is redeemed less if the `redeem()` function is called by anyone earlier, though the total amount `payout` does not change.  

Additionally, the function `RedeemHelper.redeemAll()` does not have any access control so that anyone can call it to redeem all the bonds for all the recipients.
```solidity=67
    function redeemAll( address _recipient, bool _stake ) external {
        for( uint i = 0; i < bonds.length; i++ ) {
            if ( bonds[i] != address(0) ) {
                if ( IBond( bonds[i] ).pendingPayoutFor( _recipient ) > 0 ) {
                    IBond( bonds[i] ).redeem( _recipient, _stake );
                }
            }
        }
    }
``` 

To redeem the bonds for all recipients, the `redeemAll()` function invokes the `IBond( bonds[i] )redeem()` function.
To keep this functionality, access to the `redeem()` function should also be granted to the contract `RedeemHelper`.

Lack of Access Control on `redeem()` and `redeemAll()`

Users have the possibility to send some assets in exchange for bonds within the `BondDepository` and `BondDepositoryBNB` contracts through the `deposit()` function. 
They can deposit classic ERC20 or LP tokens to get bonds.

The amount of bonds `payout` is computed as follow:
```solidity=834 
uint payout = payoutFor( value ); // payout to bonder is computed 
```
```solidity=833
uint value = ITreasury( treasury ).valueOf( principle, _amount );
```

The function called to compute `value` is `Treasury`.`valueOf()` :
```solidity=463
function valueOf( address _token, uint _amount ) public view returns ( uint value_ ) {
	if ( isReserveToken[ _token ] ) {
		// convert amount to match TEM decimals
		value_ = _amount.mul( 10 ** IERC20( TEM ).decimals() ).div( 10 ** IERC20( _token ).decimals() );
	} else if ( isLiquidityToken[ _token ] ) {
		value_ = IBondCalculator( bondCalculator[ _token ] ).valuation( _token, _amount );
	}
}
```

In case a LP is deposited (condition `isLiquidityToken[ _token ]` is met), the `IBondCalculator`.`valuation()` function is used to compute `value_` :
```solidity=288
function valuation( address _pair, uint amount_ ) external view override returns ( uint _value ) {
	uint totalValue = getTotalValue( _pair );
	uint totalSupply = IUniswapV2Pair( _pair ).totalSupply();

	_value = totalValue.mul( FixedPoint.fraction( amount_, totalSupply ).decode112with18() ).div( 1e18 );
}
```

The calculation of `value_` above depends on the `totalValue` parameter, which is computed with the `getTotalValue()` function :
```solidity=284
function getTotalValue( address _pair ) public view returns ( uint _value ) {
	_value = getKValue( _pair ).sqrrt().mul(2);
}
```
```solidity=275
function getKValue( address _pair ) public view returns( uint k_ ) {
	uint token0 = IERC20( IUniswapV2Pair( _pair ).token0() ).decimals();
	uint token1 = IERC20( IUniswapV2Pair( _pair ).token1() ).decimals();
	uint decimals = token0.add( token1 ).sub( IERC20( _pair ).decimals() );

	(uint reserve0, uint reserve1, ) = IUniswapV2Pair( _pair ).getReserves();
	k_ = reserve0.mul(reserve1).div( 10 ** decimals );
}
```

The issue is that in case of an illiquidity situation in the Pair, an attacker can manipulate `reserve0` and `reserve1` by sending tokens directly to the pair, which would end up having consequence on the amount of bound emitted by `BondDepository`/`BondDepositoryBNB`.

An example is shown below.

The pair TEM-USDT is added. This new pair is part of `isLiquidityToken` mapping within the `Treasury` contract.
To keep things simple, some assumptions are made :
- The user already owns a small amount of LP TEM-USDT;
- The two tokens have the same number of decimals;
- The `BondDepository.payoutFor( value )` function returns `value`, so the amount of `value` corresponds to the amount of bond the user will receive;
- The pool is illiquid : 10 TEM (reserve0) and 300 USDT (reserve1).

The `value` parameter, that will determine the amount of bonds, is computed as follow :
```solidity
_value = totalValue.mul( FixedPoint.fraction( amount_, totalSupply ).decode112with18() ).div( 1e18 );
totalValue = k_.sqrrt().mul(2);
k_ = reserve0.mul(reserve1).div( 10 ** decimals );
```

In this case, if a user makes a deposit, the `totalValue` would be equal to **109.54**:
```solidity
k_ = reserve0.mul(reserve1).div( 10 ** decimals ); // 3000
totalValue = k_.sqrrt().mul(2); // sqrt(3000)*2 = 109.54
```

Since reserve0 and reserve1 are low (pool is illiquid), the attacker can :
1. Send some USDT (2700 in the example) to the TEM-USDT pair.
2. Call `Pair`.`sync()` to update reserve0 (10)  and reserve1 (3000) within the pair.

Now, when the attacker deposits, the `totalValue` will be equal to **346**:
```solidity
k_ = reserve0.mul(reserve1).div( 10 ** decimals ); // 30 000
totalValue = k_.sqrrt().mul(2); // sqrt(30000)*2 = 346
```

Since the amount of bonds is decided by the following formula within `Treasury`.`valueOf()`:
```solidity
_value = totalValue.mul( FixedPoint.fraction( amount_, totalSupply ).decode112with18() ).div( 1e18 );
```

And because the attacker was able to make the `totalValue` 3 times bigger, he would get 3 times the amount of bonds he would normally get. 
Depending on the price of TEM when the user will call `BondDepository.redeem()` after the vesting period, this operation could be profitable.

Possibility to Manipulate Bonds Amount in Case of Pair Illiquidity

The implementation of the `isContract()` check can not cover all scenarios. The check can be bypassed if the call is from the constructor of a smart contract or when the contract is destroyed. Because, in that case, the codesize will also be zero.

Ineffective `isContract()` Check

The aforementioned functions  are not void-returning functions. Ignoring their return values, especially when their return values represent the status of the transaction executions, might cause unexpected exceptions.

Lack of Return Value Handling

The **require** statement can be used to check for conditions and throw an exception if the condition is not met. It is better to provide a string message containing details about the error that will be passed back to the caller.

Lack of Error Messages

The function `_beforeTokenTransfer()` and event `Transfer` in the `_mint()` function usually use the zero address as the `from` parameter, but here they use the `address(this)` instead which might bring confusions.

Wrong Address in `_mint()` Function

The `addressToString()` function, defined in multiple contracts, is never used.

Unused function `addressToString()`

The following check ensures that the `_input` variable is larger than 43200 seconds (12 hours).
However, the error message states that this check ensures that the `_input` variable is larger than 36 hours.

```solidity
require( _input >= 43200, "Vesting must be longer than 36 hours" );
```

Incoherent Error Message

When the function `setBondTerms()` is called to configure the parameters of the `Terms` structure, verifications are performed on each parameter to ensure that they have a correct value.

```solidity
function setBondTerms ( PARAMETER _parameter, uint _input ) external onlyPolicy() {
    if ( _parameter == PARAMETER.VESTING ) { // 0
        require( _input >= 43200, "Vesting must be longer than 36 hours" );
        terms.vestingTerm = _input;
    } else if ( _parameter == PARAMETER.PAYOUT ) { // 1
        require( _input <= 1000, "Payout cannot be above 1 percent" );
        terms.maxPayout = _input;
    } else if ( _parameter == PARAMETER.FEE ) { // 2
        //
    } else if ( _parameter == PARAMETER.DEBT ) { // 3
        terms.maxDebt = _input;
    } else if ( _parameter == PARAMETER.MINPRICE ) { // 4
        terms.minimumPrice = _input;
    } else if ( _parameter == PARAMETER.BCV ) { // 5
        terms.controlVariable = _input;
        adjustment.rate = 0;
    }
}
```

However, when a bond is created with the `initializeBondTerms()` function, none of those checks is performed.

```solidity
function initializeBondTerms( 
    uint _controlVariable, 
    uint _vestingTerm,
    uint _minimumPrice,
    uint _maxPayout,
    uint _maxDebt,
    uint _initialDebt
) external onlyPolicy() {
    require( currentDebt() == 0, "Debt must be 0 for initialization" );
    terms = Terms ({
        controlVariable: _controlVariable,
        vestingTerm: _vestingTerm,
        minimumPrice: _minimumPrice,
        maxPayout: _maxPayout,
        maxDebt: _maxDebt
    });
    totalDebt = _initialDebt;
    lastDecay = block.number;
}
```

Lack of Input Validation

In the contract `Ownable`, the role `owner` has authority over the following functions:
- `Ownable.renounceManagement()` to renounce the ownership of the contract;
- `Ownable.pushManagement( address newOwner_ )` to transfer ownership to a new owner with the `pull over push` model.

In contract `Treasury`, the role `owner` has authority over the following functions:
- `Treasury.auditReserves()` to update the amount of `Reserves` (value of all assets) within the contract;
- `Treasury.queue()` to queue a modification related to the contract parameters;
- `Treasury.toggle()` to trigger, after a certain delay, the parameter modification defined by `Treasury.queue()`.

The role `manager` has authority over the following functions:
- `Treasury.manage(address _token, uint _amount)` to withdraw assets if the manager is a liquidity or reserve manager.
- `Treasury.mintRewards( address _recipient, uint _amount )` to mint TEM tokens for any recipient if the manager is a reward manager.

Any compromise to these account may allow a hacker to take advantage of this authority and manipulate the project. 

Centralization Related Risks in `Treasury`

The `blocksNeededForQueue` variable indicates the period of time that needs to be respected between the initiation of a change (function `queue()`) and its realization (function `toggle()`).
The issue is that `blocksNeededForQueue` is an immutable variable, that is not checked against the value 0.

In case the `Treasury` contract is deployed with `blocksNeededForQueue` equal to 0, it would be possible to initiate a change and deploy it immediately.

Lack of Check on `blocksNeededForQueue`

In a require statement, it is unnecessary to check if a boolean variable is equal to true or false. Instead, we can use the value to do the check.  

Unnecessary Usage of Boolean Equality

In the contract `Ownable`, the role `owner` has authority over the following functions:
- `Ownable.renounceOwnership()` to renounce the ownership;
- `Ownable.transferOwnership( address newOwner_ )` to transfer ownership to a new owner.

In contract `VaultOwned`, the role `owner` has authority over the following function:
- `VaultOwned.setVault( address vault_ )` to set new vault.

In contract `TemplarToken`, the role `vault` has authority over the following function:
- `TemplarToken.mint(address account_, uint256 amount_)` to mint any amount of Templar Token to an account.

The role `owner` has has authority over the following functions:
- `TemplarToken.setMaxBuyAmount(uint256 _maxBuyAmount)` to set new maxBuyAmount;
- `TemplarToken.setAllowBuy(bool _allowBuy)` to set new allowBuy;
- `TemplarToken.setBuyDelayBlock(uint256 _buyDelayBlock)` to set new buyDelayBlock.

Any compromise to these account may allow a hacker to take advantage of this authority and manipulate the project. For instance, the hackers can use `TemplarToken.mint(address account_, uint256 amount_)` to mint any amount of Templar Token to themselves.

Centralization Related Risks  in `TemplarToken`

The change of `_owner` by function `transferOwnership()` overrides the previously set `_owner` with the new one without guaranteeing the new `_owner` is able to actuate transactions on-chain.

Pull-Over-Push Pattern

Functions that affect the status of sensitive variables should emit events as notifications to users. \
The functions below perform sensitive actions without emitting events:
- `VaultOwned.setVault()`
- `TemplarToken.setMaxBuyAmount()`
- `TemplarToken.setAllowBuy()`
- `TemplarToken.setBuyDelayBlock()`

Lack of Event Emitting

The function `TemplarToken._burnFrom()` is supposed to have private visibility for gas optimization. Otherwise, it is the same as the function `TemplarToken.burnFrom()`. 

Templar DAO

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

28

0

3

9

16