Platform: Code4rena
Start Date: 22/04/2021
Pot Size: $120,000 USDC
Total HM: 41
Participants: 10
Period: 7 days
Judge: LSDan
Total Solo HM: 28
Id: 5
League: ETH
Rank: 2/10
Findings: 42
Award: $40,849.60
🌟 Selected for report: 35
🚀 Solo Findings: 22
🌟 Selected for report: 0xRajeev
32.5901 VETH - $1,694.68
0.7822 ETH - $1,955.40
0xRajeev
ERC20 implementations are not always consistent. Some implementations of transfer and transferFrom could return ‘false’ on failure instead of reverting. It is safer to wrap such calls into require() statements to handle these failures.
The transfer call on L211 of transferOut() could be made on a user-supplied untrusted token address (from the different call sites) whose implementation can be malicious.
For reference, see similar finding from Consensys Diligence Audit of Aave Protocol V2
Manual Analysis
Use require to check the return value and revert on 0/false or use OpenZeppelin’s SafeERC20 wrapper functions.
#0 - strictly-scarce
2021-05-01T10:06:51Z
Not valid.
Since the funds came in, and did not revert, they can leave. If the call passes, then the transferout is valid.
🌟 Selected for report: 0xRajeev
32.5901 VETH - $1,694.68
0.7822 ETH - $1,955.40
0xRajeev
One of the stated protocol (review) goals is to detect susceptibility to “Any attack vectors using flash loans on Anchor price, synths or lending.” As such, USDV contract aims to protect against flash attacks using flashProof() modifier which uses the following check in isMature() to determine if currently executing contract context is at least blockDelay duration ahead of the previous context: lastBlock[tx.origin] + blockDelay <= block.number
However, blockDelay state variable is not initialized which means it has a default uint value of 0. So unless it is set to >= 1 by setParams() which can be called only by the DAO (which currently does not have the capability to call setParams() function), blockDelay will be 0 which allows current executing context (block.number) to be the same as the previous one (lastBlock[tx.origin]). This effectively allows multiple calls on this contract to be executed in the same transaction of a block which enables flash attacks as opposed to what is expected as commented on L41: "// Stops an EOA doing a flash attack in same block"
Even if the DAO can call setParams() to change blockDelay to >= 1, there is a big window of opportunity for flash attacks until the DAO votes, finalises and approves such a proposal. Moreover, such proposals can be cancelled by a DAO minority or replaced by a malicious DAO minority to launch flash attacks.
Manual Analysis
Initialize blockDelay to >= 1 at declaration or in constructor.
#0 - strictly-scarce
2021-05-01T10:15:46Z
The actual issue is simply:
blockDelay state variable is not initialized
It is intended to be initialised to 1, so this is a bug. Severity: 2
🌟 Selected for report: 0xRajeev
32.5901 VETH - $1,694.68
0.7822 ETH - $1,955.40
0xRajeev
changeDAO() is authorized to be called only from the DAO (per modifier) but DAO contract has no corresponding functionality to call changeDAO() function. As a result, DAO address cannot be changed.
Manual Analysis
Add functionality to DAO to be able to call changeDAO() of Vader.sol.
#0 - strictly-scarce
2021-05-01T10:18:44Z
#1 - dmvt
2021-05-25T13:00:53Z
Unlike #140, #157, #158, & #159 without this functionality missing functionality in the DAO becomes a very serious issue. As a result, this one is very high risk were it to be overlooked.
8.7993 VETH - $457.56
0.2112 ETH - $527.96
0xRajeev
Anyone can cancel any finalising stage proposal by simply using newProposalID equal to an existing (oldProposalID) finalising proposal of their choice. All checks (finalising, hasMinority, isEqual) will pass for the call cancelProposal(oldProposalID, oldProposalID) because the oldProposalID has minority support (L104 check) as it’s already finalising and L105 check will also pass.
There is no need to create and vote on a new proposal with newProposalID that has minority support and has the same type string as the oldProposalID, to allow the canceling of the proposal with oldProposalID.
Manual Analysis
Perform input validation on parameters to check that newProposalID != oldProposalID in cancelProposal().
#0 - strictly-scarce
2021-05-01T10:28:53Z
#1 - 0xBrian
2021-05-11T08:32:07Z
#2 - dmvt
2021-05-26T22:15:28Z
duplicate of #227
🌟 Selected for report: 0xRajeev
32.5901 VETH - $1,694.68
0.7822 ETH - $1,955.40
0xRajeev
Flash loans can significantly increase a single voter's weight and be used to impact the voting outcome. A voter can borrow a significant quantity of tokens to increase their voting weight in a transaction within which they also deterministically influence the voting outcome to their choice.
This has already happened in the case of MakerDAO governance where a flash loan was used to affect voting outcome (see https://forum.makerdao.com/t/urgent-flash-loans-and-securing-the-maker-protocol/4901) and noted by Maker team as: “a practical example for the community that flash loans can and may impact system governance”
Given that flash loans are a noted concern, the impact of it to DAO governance which can control all critical protocol parameters should be mitigated as in other places.
Manual Analysis
Account for flash loans in countMemberVotes() by using weight from previous blocks or consider capping the weight of individual voters.
#0 - strictly-scarce
2021-05-01T10:30:29Z
Not valid.
All pools use slip-based fees so flash loan attack by buying up USDV or synths is not going to work.
#1 - dmvt
2021-05-25T13:53:48Z
The funds to execute this attack do not need to come from a pool. It could be done as simply as malicious members pooling their funds in a flash loan contract, and each borrowing the funds in turn to vote.
🌟 Selected for report: 0xRajeev
32.5901 VETH - $1,694.68
0.7822 ETH - $1,955.40
0xRajeev
The internal _transfer() function is called from external facing transfer(), transferFrom() and transferTo() functions all of which have different sender addresses. It is msg.sender for transfer(), sender parameter for transferFrom() and tx.origin for transferTo(). These different senders are reflected in the sender parameter of _transfer() function. While this sender parameter is correctly used for transfer of tokens within _transfer, the call to _burn() on L129 incorrectly uses msg.sender as the burn address which is correct only in the case of the transfer() caller's context. This is incorrect for transferFrom() and transferTo() caller contexts.
This will incorrectly burn the fees from a different (intermediate contract) account for all users of the protocol interacting with the transferTo() and transferFrom() functions and lead to incorrect accounting of token balances or exceptional conditions. Protocol will break and lead to fund loss.
Manual Analysis
Change L129 to: _burn(sender, _fee);
#0 - strictly-scarce
2021-05-01T10:34:25Z
Valid, disagree with severity though. Funds-not-at-risk.
Recommend: 2
8.7993 VETH - $457.56
0.2112 ETH - $527.96
0xRajeev
As per code comments, the calcLiquidityUnits() function is supposed to calculate:
// units = ((P (t B + T b))/(2 T B)) * slipAdjustment // P * (part1 + part2) / (part3) * slipAdjustment
While part1, part2 and part3 are calculated correctly, they are combined as: uint _units = (((P * part1) + part2) / part3); which is incorrect and should be: uint _units = ((P * (part1 + part2)) / part3);
This incorrect calculation affects the liquidity calculations in addLiquidity() of Pools.sol and will lead to incorrect accounting. Protocol will break and funds will be lost.
Manual Analysis
Fix the formulae used in the calculation as per code comments by changing L239 to: uint _units = ((P * (part1 + part2)) / part3);
#0 - strictly-scarce
2021-05-01T07:43:53Z
#1 - dmvt
2021-05-26T22:19:34Z
duplicate of #204
🌟 Selected for report: 0xRajeev
32.5901 VETH - $1,694.68
0.7822 ETH - $1,955.40
0xRajeev
Incorrect initialization of timeForFullProtection to1 sec instead of 8640000 secs (100 days) as indicated in code comments appears to be a test setting mistakenly carried over for deployment. Therefore, unless timeForFullProtection is reset to 100 days by setParams() (calling this function is a missing functionality in the DAO currently), the Impermanent Loss (IL) protection "rule" of 100 days will not apply in Utils.getProtection().
This breaks a key value proposition of the Vader protocol which is IL protection as indicated in the specification: “Impermanent Loss Protection: The deposit value for each member is recorded when they deposit. When they go to withdraw, the redemption value is computed. If it is less than the deposit value, the member is paid the deficit from the reserve. The protection issued increases from 0 to 100% linearly for 100 days.”
Manual Analysis
Change to “timeForFullProtection = 8640000; //100 days” on L84.
#0 - strictly-scarce
2021-05-01T09:44:28Z
It's deliberately set to 1 second to conduct adequate testing.
14.6655 VETH - $762.61
0.352 ETH - $879.93
0xRajeev
Given the lack of input validation to check if token parameter is already listed, a malicious user can list same token multiple times to reach the anchorLimit (default=5) and prevent others from listing anchors. This will impact all anchor related accounting and logic in the protocol. Protocol will break and funds may be lost.
Manual Analysis
Check and reject if token already exists in arrayAnchors as part of input validation in listAnchor() function.
#0 - strictly-scarce
2021-05-01T09:45:49Z
Deployer will list the 5 starting stablecoins themselves, this is in the whitepaper.
#1 - dmvt
2021-05-26T22:21:46Z
duplicate of #211
14.6655 VETH - $762.61
0.352 ETH - $879.93
0xRajeev
The flipMinting() function can disable/stop conversion/redeeming of VADER<>USDV tokens upon DAO approval (when that functionality is added). When minting is disabled (i.e. false), the convert functions in USDV.sol accept VADER tokens from sender (L170) but do not burn them to mint the sender the equivalent USDV tokens. When minting is disabled (i.e. false), the redeem functions in USDV.sol accept USDV tokens from sender (L188) but do not burn them to mint the sender the equivalent VADER tokens. Both paths silently return 0 without reverting the transaction thus trapping the sent tokens and leaving the users with lost funds. Protocol will break and funds will be lost.
Manual Analysis
Revert in the paths (instead of silently returning) when minting is disabled so that tokens are not accepted for conversion or redemption.
#0 - strictly-scarce
2021-05-01T07:55:44Z
Valid, funds would be lost if minting disabled and users won't have any recourse.
#1 - strictly-scarce
2021-05-01T13:21:13Z
Recommend Severity 2 as this is minor funds loss in an edge case (compare with an exploit)
14.6655 VETH - $762.61
0.352 ETH - $879.93
0xRajeev
Incorrect initialization (perhaps testing parameterization mistakenly carried over to deployment) of secondsPerEra to 1 sec instead of 86400 secs (1 day) causes what should be the daily emission rate to be a secondly emission rate.
This causes inflation of VADER token and likely breaks VADER<>USDV peg and other protocol invariants. Protocol will break and funds will be lost.
Manual Analysis
Initialize secondsPerEra to 86400 on L67.
#0 - strictly-scarce
2021-05-01T10:17:00Z
This is purely for testing purposes.
🌟 Selected for report: 0xRajeev
9.777 VETH - $508.40
0.2346 ETH - $586.62
0xRajeev
Functions removeLiquidity() and removeLiquidityDirectly() when called directly, do not provide the the user with IL protection unlike when calling the corresponding removeLiquidity() function in Router.sol. This should be prevented, at least for removeLiquidity() or highlighted in the specification and user documentation.
Manual Analysis
Add access control (e.g. via a modifier onlyRouter) so removeLiquidity() function of Pools contract can be called only from corresponding Router contract’s removeLiquidity() function which provides IL protection. Alternatively, highlight in the specification and user documentation about which contract interfaces provide IL protection to users.
#0 - strictly-scarce
2021-05-01T11:04:21Z
User should use the Router, as designed.
🌟 Selected for report: 0xRajeev
9.777 VETH - $508.40
0.2346 ETH - $586.62
0xRajeev
Given that there are only three proposal types (GRANT, UTILS, REWARD) that are actionable, it is unclear if 'DAO' type checked in voteProposal() is a typographical error and should really be 'GRANT'. Otherwise, GRANT proposals will only require quorum (33%) and not majority (50%).
Manual Analysis
Change ‘DAO’ on L83 to ‘GRANT’ or if not, specify what DAO proposals are and how GRANT proposals should be handled with quorum or majority.
Also, check and enforce that mapPID_types are only these three actionable proposal types: GRANT, UTILS, REWARD.
#0 - strictly-scarce
2021-05-01T11:14:40Z
DAO not yet fully implemented
🌟 Selected for report: 0xRajeev
9.777 VETH - $508.40
0.2346 ETH - $586.62
0xRajeev
There is no input validation in replacePool() function to check if oldToken exists and is curated. Using a non-existing oldToken (even 0 address) passes the check on L236 (because Pools.getBaseAmount() will return 0 for the non-existing token) and newToken will be made curated. This can be used to bypass the curatedPoolLimit enforced only in curatePool() function.
Manual Analysis
Check if oldToken exists and is curated as part of input validation in replacePool() function.
#0 - strictly-scarce
2021-05-01T11:03:42Z
Valid
2.6398 VETH - $137.27
0.0634 ETH - $158.39
0xRajeev
The deploySynth() function in Pools.sol is expected to perform a check on the token parameter to determine that it is neither VADER or USDV before calling Factory’s deploySynth() function.
However, the require() incorrectly uses ‘||’ operator instead of ‘&&’ which allows both VADER and USDV to be supplied as the token parameters. This will allow an attacker to deploy either VADER or USDV as a Synth which will break assumptions throughout the entire protocol. Protocol will break and funds may be lost.
Manual Analysis
Change ‘||’ operator to ‘&&’ in the require statement: require(token != VADER && token != USDV);
#0 - strictly-scarce
2021-05-01T10:01:28Z
1.7819 VETH - $92.66
0.0428 ETH - $106.91
0xRajeev
Given the public access, this is susceptible to front-running by an attacker who can initialize this with arbitrary POOLS address before the deployer. Reinitialization will require contract redeployment because initialization can be done only once.
Reference: See finding #12 from Trail of Bits audit of Hermez Network: https://github.com/trailofbits/publications/blob/master/reviews/hermez.pdf
Manual Analysis
Use a factory pattern that will deploy and initialize atomically to prevent front-running of the initialization, or
Ensure the deployment scripts are robust in case of a front-running attack or
Given that contracts are not using delegatecall proxy pattern, it is not required to use a separate init() function to initialize parameters when the same can be done in a constructor. If the reason for doing so is to get the deployment addresses of the various contracts which may not all be available at the same time then consider rearchitecting to create a “globals” contract which can hold all the globally required addresses of various contracts. (see Maple protocol’s https://github.com/maple-labs/maple-core/blob/develop/contracts/MapleGlobals.sol for example)
#0 - strictly-scarce
2021-05-01T09:48:44Z
#1 - dmvt
2021-05-26T22:50:45Z
duplicate of #18
🌟 Selected for report: 0xRajeev
9.777 VETH - $508.40
0.2346 ETH - $586.62
0xRajeev
All the external/public functions of Pools.sol can be called by other contracts even before Pools.sol contract is initialized. This can lead to exceptions, state corruption or incorrect accounting in other contracts, which may require redeployment of contract.
Manual Analysis
Use a factory pattern that will deploy and initialize atomically to prevent front-running of the initialization, or
Given that contracts are not using delegatecall proxy pattern, it is not required to use a separate init() function to initialize parameters when the same can be done in a constructor. If the reason for doing so is to get the deployment addresses of the various contracts which may not all be available at the same time then consider rearchitecting to create a “globals” contract which can hold all the globally required addresses of various contracts. (see Maple protocol’s https://github.com/maple-labs/maple-core/blob/develop/contracts/MapleGlobals.sol for example) or
Prevent external/public functions from being called until after initialization is done by checking initialization state tracked by the inited variable.
#0 - strictly-scarce
2021-05-01T09:51:42Z
#1 - dmvt
2021-05-25T10:58:05Z
Same general comments apply to this issue as with issue #18, but it is a separate type of exploit that would be slightly less detectable. This increase in risk is balanced against the exploit being much harder to effect and the likely impact being lower.
🌟 Selected for report: 0xRajeev
9.777 VETH - $508.40
0.2346 ETH - $586.62
0xRajeev
changeDAO() updates DAO address in one-step. If an incorrect address is mistakenly used (and voted upon) then future administrative access or recovering from this mistake is prevented because onlyDAO modifier is used for changeDAO(), which requires msg.sender to be the incorrectly used DAO address (for which private keys may not be available to sign transactions).
Reference: See finding #6 from Trail of Bits audit of Hermez Network: https://github.com/trailofbits/publications/blob/master/reviews/hermez.pdf
Manual Analysis
Use a two-step process where the old DAO address first proposes new ownership in one transaction and a second transaction from the newly proposed DAO address accepts ownership. A mistake in the first step can be recovered by granting with a new correct address again before the new DAO address accepts ownership. Ideally, there should also be a timelock enforced before the new DAO takes effect.
#0 - strictly-scarce
2021-05-01T10:21:20Z
A lot has to go wrong to get to this point, so disagree with severity (funds not at risk).
Two step-process seems wise though.
#1 - dmvt
2021-05-25T13:02:37Z
Risk lowered because of the extremely low probability
4.3997 VETH - $228.78
0.1056 ETH - $263.98
0xRajeev
The conditional in calcReward() function uses the same code in both if/else parts with repeated use of reserveUSDV, reserveVADER and getUSDVAmount leading to incorrect computed value of _adjustedReserve in the else part.
This will affect harvest rewards for all users of the protocol and lead to incorrect accounting. Protocol will break and lead to fund loss.
Manual Analysis
Change variables and function calls from using USDV to VADER in the else part of the conditional which has to return the adjusted reserves when synth is not an asset i.e. an anchor and therefore base is VADER.
L144 should be changed to: uint _adjustedReserve = iROUTER(ROUTER).getVADERAmount(reserveUSDV()) + reserveVADER();
#0 - strictly-scarce
2021-05-01T08:05:22Z
Funds are not at-risk, just that some users will get less rewards, some will get more.
Recommend: 2
#1 - Mervyn853
2021-05-01T08:49:12Z
Our decision matrix for severity:
0: No-risk: Code style, clarity, off-chain monitoring (events etc), exclude gas-optimisations 1: Low Risk: UX, state handling, function incorrect as to spec 2: Funds-Not-At-Risk, but can impact the functioning of the protocol, or leak value with a hypothetical attack path with stated assumptions, but external requirements 3: Funds can be stolen/lost directly, or indirectly if a valid attack path shown that does not have handwavey hypotheticals.
Recommended: 2
🌟 Selected for report: 0xRajeev
3.259 VETH - $169.47
0.0782 ETH - $195.54
0xRajeev
The init() function initialises critical POOLS protocol address for this contract but is missing an event emission for off-chain monitoring tools to monitor this on-chain change.
Manual Analysis
Add an init event and emit that at the end of init() function.
🌟 Selected for report: 0xRajeev
3.259 VETH - $169.47
0.0782 ETH - $195.54
0xRajeev
The state variables feeOnTransfer is never initialized which leads to a default uint value of 0. When it is used on L126 in the first call to _transfer(), it will lead to a zero fee. feeOnTransfer is updated only in function _checkEmission() whose call happens later on L133, after which it has a value as calculated in that function.
This causes the only the first transfer to be a zero-fee transfer.
Manual Analysis
Initialize feeOnTransfer suitably on declaration, in constructor, or init() function.
🌟 Selected for report: 0xRajeev
3.259 VETH - $169.47
0.0782 ETH - $195.54
0xRajeev
Use of accurate comments helps read, audit and maintain code. Otherwise, it can be misleading and miss the flagging of or cause the introduction of vulnerabilities.
Misleading comment that below functions allow USDV and Synths but the code only allows Synths.
Manual Analysis
Use accurate and descriptive comments (even NatSpec) correctly describing the function behavior, parameters and return values.
0.8799 VETH - $45.76
0.0211 ETH - $52.80
0xRajeev
Given the public access, this is susceptible to front-running by an attacker who can initialize this with arbitrary assets before the deployer. Reinitialization will require contract redeployment because initialization can be done only once.
Manual Analysis
Use a factory pattern that will deploy and initialize atomically to prevent front-running of the initialization, or Ensure the deployment scripts are robust in case of a front-running attack or Given that contracts are not using delegatecall proxy pattern, it is not required to use a separate init() function to initialize parameters when the same can be done in a constructor. If the reason for doing so is to get the deployment addresses of the various contracts which may not all be available at the same time then consider rearchitecting to create a “globals” contract which can hold all the globally required addresses of various contracts. (see Maple protocol’s MapleGlobals.sol for example)
Reference: See finding #12 from Trail of Bits audit of Hermez Network: https://github.com/trailofbits/publications/blob/master/reviews/hermez.pdf
#0 - strictly-scarce
2021-05-01T08:02:01Z
Disagree that this is severe, the system is either init'd correctly or it isn't.
However the recommendation has already been taken: https://github.com/vetherasset/vaderprotocol-contracts/pull/93
#1 - Mervyn853
2021-05-01T08:38:22Z
Our decision matrix for severity:
0: No-risk: Code style, clarity, off-chain monitoring (events etc), exclude gas-optimisations 1: Low Risk: UX, state handling, function incorrect as to spec 2: Funds-Not-At-Risk, but can impact the functioning of the protocol, or leak value with a hypothetical attack path with stated assumptions, but external requirements 3: Funds can be stolen/lost directly, or indirectly if a valid attack path shown that does not have handwavey hypotheticals.
Recommended: 1
#2 - strictly-scarce
2021-05-01T09:35:26Z
Would recommend 1 instead.
#3 - dmvt
2021-05-26T22:51:02Z
duplicate of #18
0.4276 VETH - $22.24
0.0103 ETH - $25.66
0xRajeev
The changeDAO() function changes the DAO address which controls all critical protocol parameters. However, this function is missing an event emission for off-chain monitoring tools to monitor this on-chain change.
Manual Analysis
Add a changeDAO event and emit that at the end of changeDAO() function.
#0 - strictly-scarce
2021-05-01T10:22:10Z
There are plenty of events emitted as part of the normal DAO process.
#1 - dmvt
2021-05-26T22:34:55Z
duplicate of #250
0.594 VETH - $30.89
0.0143 ETH - $35.64
0xRajeev
In function getAnchorPrice, the price array of anchors is sorted and then the middle/median value is obtained. However, this assumes that the current anchorLimit of 5 will never change and so returns the 2nd index (3rd element) of the array. But this anchorLimit may be changed via setAnchorParams() in which case this function will not return the median value. This incorrect anchor price calculation will impact all accounting aspects of the protocol. Protocol will break and funds may be lost.
Manual Analysis
Determine the median value based on the length of arrayPrices instead of hardcoded value of 2.
#0 - strictly-scarce
2021-05-01T09:58:00Z
Valid, the mitigation step is correct. Although in real life the price would be median-offset, but hard to see how funds-loss or manipulation could occur, since median-offset is just as hard to manipulate as median.
Would put this at Severity 1
#1 - dmvt
2021-05-26T22:40:22Z
duplicate of #213
1.4666 VETH - $76.26
0.0352 ETH - $87.99
0xRajeev
Function transferOut is used in multiple places to transfer different tokens to recipients. However, there is no zero-address validation on the recipient address. This may accidentally transfer tokens to zero-address.
Manual Analysis
Add zero-address check in transferOut.
#0 - strictly-scarce
2021-05-01T11:05:50Z
Willl add the check, disagree with severity. Recommend: 1
#1 - dmvt
2021-05-26T22:43:17Z
duplicate of #262
🌟 Selected for report: toastedsteaksandwich
0.8799 VETH - $45.76
0.0211 ETH - $52.80
0xRajeev
ERC20 approve() is susceptible to allowance double-spend due to front-running. This is the classic ERC20 approve() race condition where a malicious spender can double-spend allowance (old and new allowance) by front-running the owner’s approve() call that aims to change the allowance.
For reference, see https://swcregistry.io/docs/SWC-114.
Manual Analysis
Use increaseAllowance() and decreaseAllowance() instead of approve().
#0 - strictly-scarce
2021-05-01T11:11:32Z
#1 - dmvt
2021-05-26T22:44:14Z
duplicate of #35
🌟 Selected for report: 0xRajeev
3.259 VETH - $169.47
0.0782 ETH - $195.54
0xRajeev
Function setRewardAddress is used by DAO to change rewardAddress from USDV to something else. However, there is no zero-address validation on the address. This may accidentally mint rewards to zero-address.
Manual Analysis
Add zero-address check to setRewardAddress.
#0 - strictly-scarce
2021-05-01T11:13:53Z
Sure, if this happened (and it wasn't intentional) then it would be voted to the correct one. Low likelihood, disagree with severity.
🌟 Selected for report: 0xRajeev
3.259 VETH - $169.47
0.0782 ETH - $195.54
0xRajeev
The default value of curatedPoolLimit only allows one curated pool at any time. This can be changed with setParams() but DAO does not have this functionality.
This will affect the scalability of the protocol and significantly limit the liquidity pool value proposition.
Manual Analysis
Change curatedPoolLimit to a higher value on L85.
#0 - strictly-scarce
2021-05-01T11:02:45Z
Intended to be 5-10 as per discussion with community. The DAO will have extra functionality to expand.
How is this medium-risk?
🌟 Selected for report: 0xRajeev
3.259 VETH - $169.47
0.0782 ETH - $195.54
0xRajeev
setParams() is authorized to be called only from the DAO (per modifier) but DAO contract has no corresponding functionality to call setParams() function. As a result, blockDelay — a critical parameter used to prevent flash attacks, is stuck with initialized value and cannot be changed.
Manual Analysis
Add functionality to DAO to be able to call setParams() of USDV.sol.
#0 - strictly-scarce
2021-05-01T10:09:40Z
#1 - dmvt
2021-05-25T12:49:56Z
This can be easily addressed by updating the DAO address on Vader, even if it is deployed this way. Low risk and impact as a result.
🌟 Selected for report: 0xRajeev
0 VETH - $0.00
0.0638 ETH - $159.60
0xRajeev
There is no need for the conditional check on L474 which wastes gas. The named-return variable curated can be simply set to _isCurated[token] or the named-return variable can be removed to simply return _isCurated[token]. This will reduce the contract size and save gas.
Manual Analysis
The named-return variable curated can be simply set to _isCurated[token] or the named-return variable can be removed to simply return _isCurated[token].
0 VETH - $0.00
0.0116 ETH - $29.09
0xRajeev
USDV address state variable is declared but never used elsewhere. This will consume an unnecessary storage slot and also increase the contract size.
Manual Analysis
Remove USDV state variable declaration on L11.
#0 - 0xBrian
2021-05-11T05:40:15Z
Not sure when it got addressed, but it did.
#1 - dmvt
2021-05-26T21:06:30Z
duplicate of #304
0 VETH - $0.00
0.0116 ETH - $29.09
0xRajeev
From Solidity’s documentation (https://docs.soliditylang.org/en/v0.8.4/contracts.html#constant-and-immutable-state-variables), “State variables can be declared as constant or immutable. In both cases, the variables cannot be modified after the contract has been constructed. For constant variables, the value has to be fixed at compile-time, while for immutable, it can still be assigned at construction time. The compiler does not reserve a storage slot for these variables, and every occurrence is replaced by the respective value. Compared to regular state variables, the gas costs of constant and immutable variables are much lower.”
The address variable POOLS can be made immutable if it is initialized at construction time within a constructor. This will avoid the use of its storage slots and lead to gas savings.
Manual Analysis
Move initialization from init() to a constructor and make address variable POOLS immutable.
#0 - dmvt
2021-05-26T21:10:21Z
duplicate of #286
0 VETH - $0.00
0.0116 ETH - $29.09
0xRajeev
init() function is never called from within the contract and so does not require public visibility. As described in https://mudit.blog/solidity-gas-optimization-tips/: “For all the public functions, the input parameters are copied to memory automatically, and it costs gas. If your function is only called externally, then you should explicitly mark it as external. External function’s parameters are not copied into memory but are read from calldata directly. This small optimization in your solidity code can save you a lot of gas when the function input parameters are huge.”
Given the one address parameters of init() function, this will save some amount of gas.
Manual Analysis
Change visibility of init() to external
#0 - 0xBrian
2021-05-11T04:39:01Z
Probably addressed in mega external patch, https://github.com/vetherasset/vaderprotocol-contracts/commit/d946b6262ac83cdb7722baa3a8684c4ceabf4ea3
#1 - dmvt
2021-05-26T21:16:03Z
duplicate of #14
🌟 Selected for report: 0xRajeev
0 VETH - $0.00
0.0638 ETH - $159.60
0xRajeev
A bool in Solidity is internally represented as a unit8 and so required only 8 bits of the 256-bits storage slot. An address variable is 160-bits. So declaring a bool next to an address variable lets Solidity pack them in the same storage slot thereby using one slot instead of two.
Moving the inited bool state variable next to one of the address state variables VADER, USDV, ROUTER or FACTORY lets the compiler pack them together in one storage slot instead of two, thereby saving one slot. It costs 20k gas to SSTORE each slot of data.
The current order where inited bool is declared before uint does not allow packing because uint itself requires the entire 256-bits of a slot, which forces the compiler to use one full slot for the inited bool variable.
For reference, see https://mudit.blog/solidity-gas-optimization-tips/
Manual Analysis
Move inited bool state variable declaration next to an address state variable declaration.
#0 - 0xBrian
2021-05-11T06:49:16Z
At some point we got rid of all the initeds.
0 VETH - $0.00
0.0172 ETH - $43.09
0xRajeev
_isMember mapping state variable is declared and used only in the getter function isMember(), but is net assigned to anywhere in the contract. This will consume an unnecessary storage slot and along with its getter function will also increase the contract size.
Manual Analysis
Remove _isMember state variable declaration on L22 and related getter function isMember().
#0 - 0xBrian
2021-05-11T04:30:07Z
0 VETH - $0.00
0.0084 ETH - $20.94
0xRajeev
From Solidity’s documentation (https://docs.soliditylang.org/en/v0.8.4/contracts.html#constant-and-immutable-state-variables), “State variables can be declared as constant or immutable. In both cases, the variables cannot be modified after the contract has been constructed. For constant variables, the value has to be fixed at compile-time, while for immutable, it can still be assigned at construction time. The compiler does not reserve a storage slot for these variables, and every occurrence is replaced by the respective value. Compared to regular state variables, the gas costs of constant and immutable variables are much lower.”
State variables name, symbol and decimals can be declared as constants and assigned at declaration (instead of constructor) because they are never modified later. This avoid 3 storage slots and associated expensive SSTOREs/SLOADs to save gas.
Manual Analysis
Declare state variables name, symbol and decimals as constant.
#0 - 0xBrian
2021-05-11T05:22:14Z
🌟 Selected for report: 0xRajeev
0 VETH - $0.00
0.0638 ETH - $159.60
0xRajeev
From Solidity’s documentation (https://docs.soliditylang.org/en/v0.8.4/contracts.html#constant-and-immutable-state-variables), “State variables can be declared as constant or immutable. In both cases, the variables cannot be modified after the contract has been constructed. For constant variables, the value has to be fixed at compile-time, while for immutable, it can still be assigned at construction time. The compiler does not reserve a storage slot for these variables, and every occurrence is replaced by the respective value. Compared to regular state variables, the gas costs of constant and immutable variables are much lower.”
The burnAddress variable can be made immutable. This will avoid the use of one storage slot and lead to gas savings.
Manual Analysis
Make burnAddress immutable.
#0 - 0xBrian
2021-05-11T05:21:50Z
🌟 Selected for report: 0xRajeev
0 VETH - $0.00
0.0638 ETH - $159.60
0xRajeev
From Solidity’s documentation (https://docs.soliditylang.org/en/v0.8.4/contracts.html#constant-and-immutable-state-variables), “State variables can be declared as constant or immutable. In both cases, the variables cannot be modified after the contract has been constructed. For constant variables, the value has to be fixed at compile-time, while for immutable, it can still be assigned at construction time. The compiler does not reserve a storage slot for these variables, and every occurrence is replaced by the respective value. Compared to regular state variables, the gas costs of constant and immutable variables are much lower.”
State variable baseline is initialized the value of _1m in constructor and then is never modified. Replacing its use directly by a constant_1m avoids a storage slot and associated SLOADs (2600 gas) leading to gas savings.
Manual Analysis
Remove baseline state variable and replace its use directly by a constant_1m
🌟 Selected for report: 0xRajeev
0 VETH - $0.00
0.0638 ETH - $159.60
0xRajeev
isEqual() helper function is never meant to be called from outside the contract and so does not require public visibility. As described in https://mudit.blog/solidity-gas-optimization-tips/: “For all the public functions, the input parameters are copied to memory automatically, and it costs gas. If your function is only called externally, then you should explicitly mark it as external. External function’s parameters are not copied into memory but are read from calldata directly. This small optimization in your solidity code can save you a lot of gas when the function input parameters are huge.”
Given the two parameters of isEqual() function, this will save some amount of gas.
Manual Analysis
Change visibility of isEqual() to internal/private
🌟 Selected for report: 0xRajeev
0 VETH - $0.00
0.0638 ETH - $159.60
0xRajeev
Instead of performing a zero-address check in moveRewardAddress on L146 or L152, it is more efficient to do so in newAddressProposal() as soon as the new address is proposed, instead of allowing a proposal for zero-address which goes through the whole voting process. If there is a requirement for zero-address proposals, it should be specified explicitly.
Depending on the participation in the voting process, this will save significant amount of gas for all the participants.
Manual Analysis
Perform input validation of zero-address in newAddressProposal() for proposedAddress parameter.
#0 - 0xBrian
2021-05-11T06:43:07Z
Not sure when it was added, but this was done.
0 VETH - $0.00
0.0116 ETH - $29.09
0xRajeev
The conditional checking if proposal has quorum in finaliseProposal() is unnecessary and will never be triggered because finalising proposals will always have quorum. Proposal without quorum are not finalised in the voteProposal() function.
Removing this code will reduce contract size and save some gas.
Manual Analysis
Remove code from L114 to L116.
#0 - 0xBrian
2021-05-11T04:25:28Z
🌟 Selected for report: 0xRajeev
0 VETH - $0.00
0.0638 ETH - $159.60
0xRajeev
POOLS address is initialized on L48 but re-initialized on L53. This unnecessary storage write will cost 200 gas because it is overwritten with the same value as earlier (see EIP-1087).
Manual Analysis
Remove re-initialization on L53.
🌟 Selected for report: 0xRajeev
0 VETH - $0.00
0.0638 ETH - $159.60
0xRajeev
repayDelay uint state variable is declared but never used elsewhere. This will consume an unnecessary storage slot and also increase the contract size.
Manual Analysis
Remove repayDelay state variable declaration on L35.
#0 - 0xBrian
2021-05-11T04:21:55Z
🌟 Selected for report: 0xRajeev
0 VETH - $0.00
0.0638 ETH - $159.60
0xRajeev
As described in https://mudit.blog/solidity-gas-optimization-tips/: “For all the public functions, the input parameters are copied to memory automatically, and it costs gas.” Changing visibility to internal/private will prevent these copies and save gas.
getILProtection() is only called from within the Router contract and so can work with internal/private visibility instead of public.
Manual Analysis
Change getILProtection() function’s public visibility to internal/private on L209
🌟 Selected for report: 0xRajeev
0 VETH - $0.00
0.0638 ETH - $159.60
0xRajeev
State variables reads using SLOADs cost 2600 gas. Instead of comparing the loop index i to a state variable arrayAnchors.length for every loop iteration, it will save gas to copy arrayAnchors.length to a memory variable for comparison.
Manual Analysis
Copy arrayAnchors.length to a memory variable outside the loop and use that for loop index comparison.
🌟 Selected for report: 0xRajeev
0 VETH - $0.00
0.0638 ETH - $159.60
0xRajeev
If the requirement is that listed anchors are unique token addresses, then the loop in replaceAnchor() can break upon match+replace to save gas from executing more loop iterations.
Manual Analysis
Add a break statement after L263.
#0 - 0xBrian
2021-05-11T05:52:24Z