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: 6/10
Findings: 10
Award: $4,794.11
🌟 Selected for report: 7
🚀 Solo Findings: 0
8.7993 VETH - $457.56
0.2112 ETH - $527.96
s1m0
A proposal can be cancelled by anyone if only exist another proposal with the same type and hasMinority() (has > 16% votes).
1 voteProposal() assume this vote trigger _finalise(). _finalise() set mapPID_finalising[_proposalID] = true 2 cancelProposal()
This works only if cancelProposal is called before finaliseProposal() but the require on line 112 helps giving the attacker at least 1 block of advantage because cancelProposal() doesn't have requirement of time and can be called in the same block of step 1.
Manual analysis
Rethink the design of cancelProposal(). One idea that came to my mind would be to have more than 1 person to call cancelProposal() to be valid e.g. 1% of the votes for the newProposal + a number of different address who voted in the newProposal because 1 address could have 1% of the votes.
#0 - strictly-scarce
2021-05-01T07:30:57Z
#1 - 0xBrian
2021-05-11T08:32:22Z
#2 - dmvt
2021-05-26T22:15:25Z
duplicate of #227
8.7993 VETH - $457.56
0.2112 ETH - $527.96
s1m0
The formula doesn't coincide with the comment.
https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Utils.sol#L266
Manual analysis.
Correct the code or the comment.
#0 - dmvt
2021-05-26T22:17:56Z
duplicate of #214
8.7993 VETH - $457.56
0.2112 ETH - $527.96
s1m0
The formula is different from the comment, the impact could be high if the comment was actually rigth.
https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Utils.sol#L239 is different from comment on line 234. Should be _units = ((P * (part1 + part2)) / part3) if the comment is rigth.
Manual analysis
Correct the code or the comment.
#0 - dmvt
2021-05-26T22:19:37Z
duplicate of #204
2.6398 VETH - $137.27
0.0634 ETH - $158.39
s1m0
The mapPID_finalised is set on completeProposal() but voteProposal() doesn't check it. A malicious user with enough capital and/or together with other people could execute a proposal in his favor (e.g. a grant) infinitely.
https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/DAO.sol#L131 1 newGrantProposal() 2 voteProposal() until enough votes 3 finaliseProposal() -> grantFunds() -> completeProposal() 4 repeat
Manual analysis.
In voteProposal() require(!mapPID_finalised[proposalID], "Proposal already executed")
#0 - strictly-scarce
2021-05-01T13:25:13Z
If they have this weighting in the DAO, then they can pass another proposal of same nature.
GRANT funding is rate-limited for this reason.
#1 - strictly-scarce
2021-05-01T13:25:59Z
However seems wise to add this check to force a new proposal
#2 - strictly-scarce
2021-05-01T13:28:32Z
#3 - 0xBrian
2021-05-11T04:27:32Z
Unused mapPID_finalised addressed https://github.com/vetherasset/vaderprotocol-contracts/commit/6f961e6cd159e905ef53a5a067f956d21f8a46ee
#4 - dmvt
2021-05-26T22:46:29Z
duplicate of #229
0.4276 VETH - $22.24
0.0103 ETH - $25.66
s1m0
Events not emitted for important state changes. https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Router.sol#L93 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Router.sol#L98 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Router.sol#L196 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Router.sol#L201 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vault.sol#L61 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vader.sol#L163 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vader.sol#L171 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vader.sol#L179 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vader.sol#L184 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vader.sol#L188 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vader.sol#L193 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vader.sol#L198
Manual analysis.
Emit events with meaningful names for the changes made.
#0 - strictly-scarce
2021-05-01T13:04:12Z
The ethereum state machine isn't a parking lot for event data
setParams() - no, plenty events in DAOsetAnchorParams() - no, plenty events in DAOBut these are warranted, purely for off-chain metrics:
addDepositData() - valid for off-chain IL trackingremoveDepositData() - valid for off-chain IL tracking#1 - strictly-scarce
2021-05-01T13:04:46Z
Severity: 0, no impact to protocol itself
1.4666 VETH - $76.26
0.0352 ETH - $87.99
s1m0
The blockDelay variable is not set so at deploy time the flashProof() modifier doesn't work as expected.
https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/USDV.sol#L41
Manual analysis
Set the blockDelay in the init() with a predefined value or through an argument.
#0 - dmvt
2021-05-24T13:17:44Z
duplicate of #307
1.4666 VETH - $76.26
0.0352 ETH - $87.99
s1m0
The token can be sent to address(0) through a normal transfer() without decreasing the totalSupply as it would with calling burn() and it could cause an unintentional burn.
https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vader.sol#L122 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/USDV.sol#L102
Manual analysis
Consider checking the recipient address to be != address(0).
🌟 Selected for report: s1m0
3.259 VETH - $169.47
0.0782 ETH - $195.54
s1m0
voteProposal() doesn't check that proposalID <= proposalCount.
https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/DAO.sol#L79
Manual analysis
in voteProposal() require(proposalID <= proposalCount, "Proposal not existent") should be <= because proposalCount is updated before using it (e.g. https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/DAO.sol#L59) in this way the proposal n. 0 is not assignable i'm not sure if it's wanted or not.
🌟 Selected for report: toastedsteaksandwich
0.8799 VETH - $45.76
0.0211 ETH - $52.80
s1m0
Vader, USDV and VETH are vulnerable to double-spend allowance attack in which an attacker can front-run the execution of an approve() function.
Manual analysis
Consider implementing increaseAllowance() and decreaseAllowance().
#0 - strictly-scarce
2021-05-01T13:32:03Z
hypothetical
#1 - dmvt
2021-05-26T22:44:25Z
duplicate of #35
0 VETH - $0.00
0.0116 ETH - $29.09
s1m0
Some variables are declared but never used.
https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Factory.sol#L10 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Factory.sol#L11 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Router.sol#L48 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Router.sol#L52 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Pools.sol#L22
Manual analysis
Remove them.
#0 - 0xBrian
2021-05-11T06:16:18Z
We got rid of all those.
#1 - dmvt
2021-05-26T21:06:37Z
duplicate of #304
0 VETH - $0.00
0.0116 ETH - $29.09
s1m0
The following functions can be declared external.
Vader.sol
Slither
Declare them external. Use slither in your CI it helps not only for this.
#0 - 0xBrian
2021-05-11T04:37:14Z
Probably addressed in mega external patch, https://github.com/vetherasset/vaderprotocol-contracts/commit/d946b6262ac83cdb7722baa3a8684c4ceabf4ea3
#1 - dmvt
2021-05-26T21:16:00Z
duplicate of #14
0 VETH - $0.00
0.0084 ETH - $20.94
s1m0
Some variables can be declared constant reducing the gas cost. Vader.sol
Manual analysis
Make them constant.
#0 - 0xBrian
2021-05-11T05:21:20Z
#1 - dmvt
2021-05-20T20:29:27Z
duplicate of #129
0 VETH - $0.00
0.0172 ETH - $43.09
s1m0
The logic of the following functions can be simplified reducing the gas cost.
https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vader.sol#L163 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vader.sol#L171
Manual analysis
function flipEmissions() external onlyDAO { emitting = !emitting; }
same thing with flipMinting.
#1 - dmvt
2021-05-26T22:06:17Z
duplicate of #197
0 VETH - $0.00
0.0287 ETH - $71.82
s1m0
The following functions check that an uint > 0 but it's always true.
https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Utils.sol#L278 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Utils.sol#L197 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vader.sol#L127
Manual analysis
Remove the checks.
#0 - 0xBrian
2021-05-05T04:34:02Z
Two of those really were tautologies. Checking uint >= 0 really is needless. But checking uint > 0 is OK.
#1 - 0xBrian
2021-05-11T05:58:19Z
At some point we got rid of all the uint >= 0 tautological checks.
🌟 Selected for report: s1m0
0 VETH - $0.00
0.0638 ETH - $159.60
s1m0
The function getSynth(token) in the following functions is called multiple times instead of saving the result.
https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Pools.sol#L143 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Pools.sol#L155 https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Pools.sol#L167
Manual analysis
Save the result.
🌟 Selected for report: s1m0
0 VETH - $0.00
0.0638 ETH - $159.60
s1m0
The following function is unnecessarily complicated.
https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/USDV.sol#L195
Manual analysis
The token argument can be omitted due to the fact the function is internal and only called with VADER. The outer if can be omitted.
🌟 Selected for report: s1m0
0 VETH - $0.00
0.0638 ETH - $159.60
s1m0
voteProposal can be simplified,
https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/DAO.sol#L83
Manual analysis
if(hasMajority(proposalID) && (isEqual(_type, 'DAO') || isEqual(_type, 'UTILS') || isEqual(_type, 'REWARD'))) check the condition on line 84 in the outer if.
0 VETH - $0.00
0.0116 ETH - $29.09
s1m0
In finaliseProposal the code between 114-116 can be omitted because it's always true from the require on line 113. mapPID_finalising[proposalID] is set to true only in _finalise() and _finalise() is called only if the proposal has quorum (line 85 - 88).
https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/DAO.sol#L114
Manual analysis
Remove the code between 114-116
#0 - 0xBrian
2021-05-11T04:25:12Z
#1 - dmvt
2021-05-26T21:57:09Z
duplicate of #186