dforce-network / dtoken Goto Github PK
View Code? Open in Web Editor NEWdForce yield token
License: MIT License
dForce yield token
License: MIT License
Severity: Informational
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");
Properly initialize the ReentrancyGuard
as well when initializing the contracts.
Severity: Low
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.
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
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.
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
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.
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:
0x885dd179c76ee5949b9053f1958ba3a91e4cf592
, proxy for 0x3c573234e1a9e47e0cac56db543e1ca153eb7f8b0x8916a9b0064feab04b3bf3729adbb0be119ed12d
, proxy for 0xbde39e19dc806620d5377cd14827d42bbe42c8f9, whose underlying USR address is 0xef004c5cdfaab19299b3ed66f14ec010fe5f20d0
, proxy for 0xd4eef282f58ecaf12118e96ed4c06f60f88009c6, which NEED decompileThe 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
Lines 357 to 377 in 563d7d0
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:
Thanks for your answering~
Severity: Informational
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,
On BSC, the the similar action could be triggered, since dToken connects to CREAM.finance, and CREAM token could be collected.
Severity: Low
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.
Add an explicit check to updateProportions
to check for duplicate Handlers.
Severity: Low
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.
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
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.
Correct the type of the decimals
variable to a uint8
.
Severity: Undetermined
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.
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
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"
);
}
Add a check for duplicate Handlers to setHandler
.
Severity: Informational
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.
Consider participating in the Aave referral program and integrating your referral code into the dToken system.
Severity: Code Quality
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
, andredeem(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
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.
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
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.
Avoid using the CompoundHandler while a long term solution to support airdrops is investigated and implemented.
Severity: Code Quality
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
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
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)
Check for this type of behavior when considering supporting new assets and avoid the use of deflationary tokens.
Severity: Code Quality
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
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.