dforce-network / dtoken Goto Github PK

Severity: Informational

Description

ReentrancyGuard was updated to reflect a newer version of OpenZeppelin, which moves from bool notStarted to this:

    uint256 private constant _NOT_ENTERED = 1;
    uint256 private constant _ENTERED = 2;

    uint256 private _status;

    constructor() internal {
        _status = _NOT_ENTERED;
    }

However, _status is no longer being set in the initialize function anymore.

modifier nonReentrant() {
        // On the first call to nonReentrant, _notEntered will be true
        require(_status != _ENTERED, "ReentrancyGuard: reentrant call");

Recommendation

Properly initialize the ReentrancyGuard as well when initializing the contracts.

Severity: Low

Description

DToken's approve function is missing a whenNotPaused modifier. The other entry points for users have the modifier so it appears to be an oversight to omit it from this function.

DToken.approve(address,uint256) (contracts/DToken.sol#866-870) should use the Pausable modifier. It changes:
        -DToken.allowance (contracts/DToken.sol#42)

This issue was identified by Slither.

Recommendation

Short term, add the missing whenNotPaused modifier.

Long term, use crytic.io—Crytic catches this bug. This will help ensure that vulnerabilities are not introduced into the codebase.

add 404 page
eg:https://testdtoken.dforce.network/aaa

Severity: Informational

Description

The transferFrom function used by DToken, mimicking the token implementation of Dai, has a special case in the allowance check such that if a user's allowance is set to 2^256-1 (the max unsigned integer), the allowance is considered to be infinite and never decreases unless modified explicitly by the allower. Though this behavior does not introduce security concerns we are aware of, it diverges from the expected ERC20 behavior and it should be documented explicitly so that users are aware of it and do not accidentally trigger this behavior.

Recommendation

Short term, consider whether this feature is necessary to include. If so outline the infinite allowance behavior in user documentation and the project README.

Long term, always document any deviations from a specification to help facilitate compatibility.

Add support to AAVE V2, just implement a handler to connect with AAVE V2 like the handler connected AAVE did.

Severity: Medium

Description

Authenticated users can exert significant control over the system. For example, a malicious insider with access to the deployment wallet could have the ability to siphon funds by adding a new attacker-controlled address as a handler and rebalancing dToken contracts to move tokens to this address. This degree of centralization also requires users to place a lot of trust in dForce that they will act in their best interests.

Recommendation

Renounce ownership from the deployment wallet and ensure only multi-sig wallets are authorized to the current system.

Long term, investigate potential methods of decentralizing the system.

Info: current mainnet eth block number: 11210219

I'm wondering where the interest come from, so I made some investigations for dUSDx:

The contract address for dUSDx is 0x109917f7c3b6174096f9e1744e41ac073b3e1f72 etherscan, and it has two handlers:

• InternalHandler deployed at 0x885dd179c76ee5949b9053f1958ba3a91e4cf592, proxy for 0x3c573234e1a9e47e0cac56db543e1ca153eb7f8b
• USRHandler deployed at 0x8916a9b0064feab04b3bf3729adbb0be119ed12d, proxy for 0xbde39e19dc806620d5377cd14827d42bbe42c8f9, whose underlying USR address is 0xef004c5cdfaab19299b3ed66f14ec010fe5f20d0, proxy for 0xd4eef282f58ecaf12118e96ed4c06f60f88009c6, which NEED decompile

The code for calculating exchange ratio, namely how many USDx can 1 dUSDx exchange, is getCurrentExchangeRate, which is implemented by summing up the handlers' getRealBalance(_token) and then divided by totalSupply

By calling getBaseData for different blocks (using MetaMask API), I find that getCurrentExchangeRate update for each block:

def getbasedata(id, blockid):
    data = '{"id":%d,"jsonrpc":"2.0","method":"eth_call","params":[{"data":"0x148a95ea","to":"0x109917f7c3b6174096f9e1744e41ac073b3e1f72"},"%s"]}'%(id, blockid)
    response = sess.post('https://mainnet.infura.io/v3/9aa3d95b3bc440fa88ea12eaa4456161', headers=headers, data=data)
    res = response.json()
    #print(res)
    data = base64.b16decode(res["result"][2:].upper())
    i = 1
    rate = int.from_bytes(data[i*32:i*32+32],"big")
    return rate

>>> getbasedata(1,hex(11210217))
1008824037259265003
>>> getbasedata(1,hex(11210218))
1008824072639567928
>>> getbasedata(1,hex(11210219))
1008824108020446490

The InternalHandler's getRealBalance can be easily called via etherscan dapp getRealBalance 0xeb269732ab75a6fd61ea60b06fe994cd32a83549

And this output seems stable 83373969497199741501815, not change for hours. So this is not the source of interest.

However, I cannot find way to call USRHandler's getRealBalance, and the implementation code is not available on Etherscan.

So, my question is:

• What is USR Token?
• Where can I find the implementation of USR Token, i.e. the code for contract 0xd4eef282f58ecaf12118e96ed4c06f60f88009c6
• Where does the interest come from, and why this is updating for every block? I guess the calculation involves block number?

Thanks for your answering~

Severity: Informational

Description

Aave interest is defined as the difference between the current balance (the result of a call to balanceOf) and the principal balance (the result of a call to principalBalanceOf). The difference is that balanceOf will return theoretical, but not-yet-minted ATokens.

    function _updateInterest(address _underlyingToken)
        internal
        returns (uint256)
    {
        uint256 _balance = getBalance(_underlyingToken);

        // Interest = Balance - UnderlyingBalance.
        uint256 _interest = _balance.sub(
            getUnderlyingBalance(_underlyingToken)
        );

        // Update the stored interest
        interestDetails[_underlyingToken] = interestDetails[_underlyingToken]
            .add(_interest);

        return _balance;
    }

However, theoretical tokens are minted (through calling cumulateBalanceInternal) upon any action such as a transfer or an interest stream redirection. As such, a user can trick the Aave handler into believing that no interest was generated.

The interest data isn't currently used on-chain, so it may not be a serious issue.

Refer to COMP collected from compoud.finance,

1. collect COMP when the amount available to collect is large enough, say 10,
2. sell them by AMM (uniswap),
3. put underlying asset into internal pool,
   so the profit will go to dToken holders.

On BSC, the the similar action could be triggered, since dToken connects to CREAM.finance, and CREAM token could be collected.

Severity: Low

Description

For example, imagine there are four handlers each with weight 250000. Now the proportions are updated with handlers [handler1, handler1, handler1, handler1] and values [1000000,0,0,0]

 function setHandlers( 
     address[] memory _handlers, 
     uint256[] memory _proportions 
 ) private { 
     require( 
         _handlers.length == _proportions.length && _handlers.length > 0, 
         "setHandlers: handlers & proportions should not have 0 or different lengths" 
     ); 
  
     // The 1st will act as the default handler. 
     defaultHandler = _handlers[0]; 
  
     uint256 _sum = 0; 
     for (uint256 i = 0; i < _handlers.length; i++) { 
         require( 
             _handlers[i] != address(0), 
             "setHandlers: handlerAddr contract address invalid" 
         ); 
  
         _sum = _sum.add(_proportions[i]); 
  
         handlers.push(_handlers[i]); 
         proportions[_handlers[i]] = _proportions[i]; 
         isHandlerActive[_handlers[i]] = true; 
     } 
  
     // The sum of proportions should be 1000000. 
     require( 
         _sum == totalProportion, 
         "the sum of proportions must be 1000000" 
     ); 
 } 

This will cause handler1 to have a weight of 0 and the system to have a total weight of only 750000 (a similar scenario could result in a weight sum that is greater than 1000000). However, the sanity check for the sum at the end of the function will not revert as the sum it calculates is across the _proportions input array, not the actual state of the proportions mapping at the end of the update.

Recommendation

Add an explicit check to updateProportions to check for duplicate Handlers.

Severity: Low

Description

Several third-party contracts are copy-pasted into the repository, including several OpenZeppelin contracts as well as some from DappHub. Moreover, the code documentation does not specify the exact revision that was used and if it was modified. This makes receiving updates and security fixes on these dependencies unreliable as they must be updated manually.

Recommendations

Short term, review the codebase and document the source and version used of each dependency. Include the third-party sources as submodules in your Git repository so that internal path consistency can be maintained and dependencies are updated periodically.

Long term, use an Ethereum development environment and NPM to manage packages as part of your project.

Severity: Informational

Description

According to the EIP-20 standard, if implemented, the decimals function should return a uint8. However, as the decimals state variable in the DToken contract is declared as a uint256, the auto-generated getter function will return a uint256 instead of the expected type.

Recommendation

Correct the type of the decimals variable to a uint8.

Severity: Undetermined

Description

dToken has enabled optional Solidity compiler optimizations in its Truffle config file:

 // Configure your compilers 
 compilers: { 
     solc: { 
         version: "0.5.12", // Fetch exact version from solc-bin (default: truffle's version) 
         // docker: true,        // Use "0.5.1" you've installed locally with docker (default: false) 
         settings: { // See the solidity docs for advice about optimization and evmVersion 
             optimizer: { 
                 enabled: true, 
                 runs: 200 
             }, 
             //  evmVersion: "byzantium" 
             // } 
         } 
     } 

There have been several bugs with security implications related to optimizations. Moreover, optimizations are actively being developed. Solidity compiler optimizations are disabled by default. It is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised.

High-severity security issues due to optimization bugs have occurred in the past. A high-severity bug in the emscripten-generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6.

A compiler audit of Solidity from November, 2018 concluded that the optional optimizations may not be safe. Moreover, the Common Subexpression Elimination (CSE) optimization procedure is “implemented in a very fragile manner, with manual access to indexes, multiple structures with almost identical behavior, and up to four levels of conditional nesting in the same function.” Similar code in other large projects have resulted in bugs.

There are likely latent bugs related to optimization, and/or new bugs that will be introduced due to future optimizations.

Recommendation

Short term, measure the gas savings from optimizations, and carefully weigh that against the possibility of an optimization-related bug.

Long term, monitor the development and adoption of Solidity compiler optimizations to assess its maturity.

Severity: Low

Description

This is similar to #14.

Calls to setHandler do not explicitly check that input array does not contain duplicate Handler contract addresses. The system appears to handle this gracefully in most cases. However, if the default handler is specified multiple times, it may fail loudly. Regardless, it is clear from the implementation in addHandlers that duplicate handlers should not be allowed.

 function setHandlers( 
     address[] memory _handlers, 
     uint256[] memory _proportions 
 ) private { 
     require( 
         _handlers.length == _proportions.length && _handlers.length > 0, 
         "setHandlers: handlers & proportions should not have 0 or different lengths" 
     ); 
  
     // The 1st will act as the default handler. 
     defaultHandler = _handlers[0]; 
  
     uint256 _sum = 0; 
     for (uint256 i = 0; i < _handlers.length; i++) { 
         require( 
             _handlers[i] != address(0), 
             "setHandlers: handlerAddr contract address invalid" 
         ); 
  
         _sum = _sum.add(_proportions[i]); 
  
         handlers.push(_handlers[i]); 
         proportions[_handlers[i]] = _proportions[i]; 
         isHandlerActive[_handlers[i]] = true; 
     } 
  
     // The sum of proportions should be 1000000. 
     require( 
         _sum == totalProportion, 
         "the sum of proportions must be 1000000" 
     ); 
 } 

Recommendation

Add a check for duplicate Handlers to setHandler.

Severity: Informational

Description

The Aave system supports the use of referral codes to participate in a fee share structure. As the fees are being collected regardless, it may be in dForce's best interest to specify a referral code so that the fee share can be redirected back into the dForce ecosystem.

Recommendation

Consider participating in the Aave referral program and integrating your referral code into the dToken system.

Severity: Code Quality

Description

There are two functions which can be used to redeem dTokens:

• burn(address src, uint256 wad), which will burn wad amount of dTokens from src and transfer the unlocked assets to msg.sender, and
• redeem(address src, uint256 pie), which will unlock pie assets and burn the corresponding amount of dTokens required from src, before transferring the assets to msg.sender.

Which function operates on dTokens vs. underlying is not immediately clear. A more common naming convention would be operation() and operationUnderlying() (i.e., change burn() to redeem() and redeem() to redeemUnderlying()).

Additionally, burn() and redeem() share a lot of code and it may be beneficial to break these up into internal sub-functions for clarity and consistency when modifying redemption behavior.

Severity: Informational

Description

In order to mitigate the well-known ERC20 race condition, some tokens disallow approvals being set to non-zero values when there is already an existing allowance set. Some well-known tokens that take this approach are USDT, ANT, SNT and any other token built from a MiniMeToken. Unfortunately, though this does help to address the race condition, this behavior violates the expected ERC20 behavior. As a result, calls to approve() in the *Handler contracts that interact with these tokens will succeed the first time they are called and fail on every subsequent call. This function does not take a parameter for the actual value to set the allowance to, and instead attempts to re-set the allowance to MAX_UINT256 on every call.

This issue is not likely to be practically exploitable, as the quantity of transfers required to exhaust an allowance this high is not practical for most tokens. However, it should still be fixed.

Recommendation

Modify the function parameters to take a specific allowance amount as a variable instead of using MAX_UINT256 universally.

Design an automatic rebalance strategy - For example, when depositing X amount of USDT into dUSDT, the X will be distributed to different underlying lending protocols based on best saving interests. Considering gas consumption, the rebalancing should not be triggered upon every single deposit/withdraw action, there should be a range of internal pool acted as a buffer, say +- 5% (internal pool holds 10% underlying asset), so within 5%~15%, the rebalancing should not be trggered.

Severity: Code Quality

Description

The convention followed in the rest of the system is event VarChanged(var oldVal, var newVal) but when updating the dispatcher, the values are reversed: event NewDispatcher(address Dispatcher, address oldDispatcher);

Severity: Medium

The CompoundHandler contract will be a frequent recipient of COMP airdrops as it is the address that directly interfaces with the Compound system. However, as the contract has no facilities to track end-user shares of these unexpected tokens or to support their withdrawal directly by users, the tokens will be effectively lost.

In an emergency, it may be possible to recover them by authorizing an EOA or by tricking the system into thinking COMP is a cToken. However, these approaches would be incredibly manually-intensive, centralized and error-prone and should be avoided.

Recommendation

Avoid using the CompoundHandler while a long term solution to support airdrops is investigated and implemented.

Severity: Code Quality

Description

Origination fees are calculated based on the function call that triggers it. However, redeem() is intended to share the same fee as calls to burn(). This function identifier is hardcore into the contract:
_redeemLocal.originationFee = originationFee[0x9dc29fac];

The use of magic numbers such as this should be avoided and this constant could be replaced by DToken(this).burn.selector for improved clarity.

Severity: Code Quality

Description

The DTokenProxy contract is an implementation of the OpenZeppelin AdminUpgradeabilityProxy. The following lines have been commented out from that parent contract:

modifier ifAdmin() { 
    if (msg.sender == _admin()) { 
        _; 
    } /* else { 
    _fallback(); 
    }*/ 
} ```

  // require(msg.sender != _admin(), "Cannot call fallback function from the proxy admin"); 

By having the _fallback() call included in the modifier, it allows the implementations to define functions such as admin() and pendingAdmin(). Currently, these functions would be captured by the proxy instead of being delegated to the underlying implementation.

The reason for this modification is unclear. If it provides some benefit, this should be documented. Otherwise introducing modifications like this can have unintended side effects.

Severity: Informational

Description

Deflationary tokens don't send the full expected amount to the destination on calls to transfer/transferFrom, which can result in incorrect bookkeeping. The DToken contract may be affected if this type of token were used due to the following sanity check:

 require( 
     IHandler(_withdraw[i]).withdraw(_token, _withdrawAmount[i]) == 
         _realWithdrawAmount[i], 
     "rebalance: actual withdrown amount does not match the wanted" 
 ); 

It's not a severe issue right now, but it could result in the system not holding enough tokens for users' positions and they may not receive the expected amount of tokens out.

Note that some high-profile tokens are (or can become) deflationary. For example, USDT could take a fee on transfers, though it is not currently enabled.

This behavior may be intentional, or this check might just be a sanity check slapped on there without really thinking about the implications. Given the recent fiasco revolving around deflationary tokens though, might as well mention it. Tether is technically deflationary, after all (although currently the fee is set to zero)

Recommendation

Check for this type of behavior when considering supporting new assets and avoid the use of deflationary tokens.

Severity: Code Quality

Description

The following events in the DToken contract would benefit from having an indexed field for easier off-chain discovery.

•    event Mint( 
     address account, 
     uint256 pie, 
     uint256 wad, 
     uint256 totalSupply, 
     uint256 exchangeRate 
  

•   event Burn( 
     address account, 
     uint256 pie, 
     uint256 wad, 
     uint256 totalSupply, 
     uint256 exchangeRate 
  

•    event Rebalance( 
     address admin, 
     address[] withdraw, 
     uint256[] withdrawAmount, 
     address[] supply, 
     uint256[] supplyAmount 
  

•    event TransferFee( 
     address admin, 
     address token, 
     address feeRecipient, 
     uint256 amount  
  

•    event NewDispatcher(address Dispatcher, address oldDispatcher);
  

•    event NewOriginationFee( 
     bytes4 sig, 
     uint256 oldOriginationFeeMantissa, 
     uint256 newOriginationFeeMantissa