Vader Protocol contest - JMukesh's results

Capital efficient liquidity protocol

General Information

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

Vader Protocol

Findings Distribution

Researcher Performance

Rank: 9/10

Findings: 4

Award: $2,247.13

🌟 Selected for report: 3

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Vader Protocol

Findings Information

🌟 Selected for report: jvaqa

Also found by: JMukesh

Labels

bug

duplicate

disagree with severity

3 (High Risk)

sponsor disputed

Awards

14.6655 VETH - $762.61

0.352 ETH - $879.93

External Links

Link to GitHub issue

Handle

JMukesh

Vulnerability details

Impact

its impact will be high since, collecting fee from user is one of the source of revenue. If user are able to bypass the fees collected by protocol then it will decrease the revenue

Proof of Concept

In Vether.sol https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vether.sol#L93

function addExcluded(address excluded) public { mapAddress_Excluded[excluded] = true; }

this function can be called by anyone , due to which anyone can their map their own address. Since this address will be excluded, because of that _getFee(address, address, uint){} will return 0 fees which leads to bypass of transaction fees when it call transfer(address, uint){} function.

Tools Used

No tool used

Recommended Mitigation Steps

Add access control modifier

#0 - Mervyn853
2021-04-25T00:00:16Z

strictly-scarce — 4/24/2021 at 10:33 PM Regarding Vader-Review, just FYI an older, incorrect, copy of Vether.sol was put into the repo. (it has some small variations in order to simplify testing code for Vader, which is the focus of the Repo).

The correct code deployed on Mainent () is available here: https://etherscan.io/address/0x4Ba6dDd7b89ed838FEd25d208D4f644106E34279#code

The incorrect, and not applicable, testing contract that was uploaded is here: https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vether.sol

#1 - strictly-scarce
2021-04-25T09:49:30Z

The reason this deviation is for ease of testing in how the testing framework executes.

The correct deployed Vether contract does not have this issue.

REcommend: Close

#2 - Mervyn853
2021-05-01T08:48:29Z

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: 0

#3 - dmvt
2021-05-26T19:04:37Z

The warden should be paid out on this issue, in my opinion, because the code was included in the repo to be reviewed. The work to review the contract was done despite the fact that the team has addressed the issue and has already deployed vether.sol. I do not think that any issues related to Vether.sol should be included in the final report generated by @code423n4.

#4 - dmvt
2021-05-26T22:25:20Z

duplicate of #189

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Auditors

Browse

Contests

Browse

Get in touch

Contact Twitter

Findings Information

🌟 Selected for report: s1m0

Also found by: 0xRajeev, JMukesh, a_delamo, cmichel

Labels

bug

duplicate

1 (Low Risk)

Awards

0.4276 VETH - $22.24

0.0103 ETH - $25.66

External Links

Link to GitHub issue

Handle

JMukesh

Vulnerability details

Impact

function changeDAO(address newDAO) external onlyDAO { require(newDAO != address(0), "address err"); DAO = newDAO; }

It has no event, so it is difficult to track off-chain newDao changes.

Proof of Concept

In vader.sol https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vader.sol#L193

Tools Used

No tool used

Recommended Mitigation Steps

add event for changing dao adress

#0 - dmvt
2021-05-26T22:35:08Z

duplicate of #250

Findings Information

🌟 Selected for report: JMukesh

Also found by: 0xRajeev, a_delamo, s1m0

Labels

bug

G (Gas Optimization)

sponsor confirmed

addressed

Awards

0 VETH - $0.00

0.0116 ETH - $29.09

External Links

Link to GitHub issue

Handle

JMukesh

Vulnerability details

Impact

public functions that are never called by the contract should be declared external to save gas.

Proof of Concept

In Vault.sol -- > init() and grant()

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vault.sol#L45

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vault.sol#L68

Vader.sol -- > burn()

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vader.sol#L146

Utils.sol -- > init(), getProtection()

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Utils.sol#L30

Router.sol -- >

init(address,address,address) getVADERAmount(uint256) getUSDVAmount(uint256) borrow(uint256,address,address) repay(uint256,address,address) checkLiquidate() getSystemCollateral(address,address) getSystemDebt(address,address) getSystemInterestPaid()

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Router.sol#L77

Pools.sol

init(address,address,address,address) isMember(address) isSynth(address)

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Pools.sol

Dao.sol

init(address,address,address) newGrantProposal(address,uint256) newAddressProposal(address,string) voteProposal(uint256) cancelProposal(uint256,uint256) finaliseProposal(uint256)

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/DAO.sol#L46

Tools Used

slither

Recommended Mitigation Steps

use external instead of public visibility to save gas

#0 - strictly-scarce
2021-04-25T09:58:16Z

Valid, will action this.

#1 - 0xBrian
2021-05-11T06:45:37Z

Most likely fixed in mega external patch. https://github.com/vetherasset/vaderprotocol-contracts/commit/d946b6262ac83cdb7722baa3a8684c4ceabf4ea3#diff-f5afe9240d284bf89e33b79c68c1ab21a6c18323c11d480d25144c18b8a9373a

Findings Information

🌟 Selected for report: JMukesh

Labels

bug

disagree with severity

1 (Low Risk)

sponsor disputed

Awards

3.259 VETH - $169.47

0.0782 ETH - $195.54

External Links

Link to GitHub issue

Handle

JMukesh

Vulnerability details

Impact

when anyone try to remove liquidity or wanted swap , their transaction may get failed. Even though transaction got failed , event will be emitted which can be problematic if we are keeping track record offchain

Proof of Concept

In Pools.sol

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Pools.sol#L92

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Pools.sol#L101

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Pools.sol#L163

in _removeLiquidity(){} function, swap(){} function, burnSynth(){} function, event is emitted before transferOut(){} function get completed , since transferOut(){} function does not check return value from transfer

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Pools.sol#L211

transaction may get failed even though event is emitted

Tools Used

No tool used

Recommended Mitigation Steps

check return value from transfer function in order to know whether transaction got successfully executed or not

#0 - strictly-scarce
2021-04-25T09:47:57Z

Events are not used for tracking state. If a tx fails, the contract's storage will not update. The contract state is the source of truth.

Recommend: Close, no issue found.

#1 - Mervyn853
2021-05-01T08:47:45Z

Our decision matrix for severity:

Recommended: 0

#2 - dmvt
2021-05-26T19:08:38Z

This can and will cause cascading issues for third party dapps. At worst, completely hypothetically, this could cause a user to act as if they have funds they don't or vise versa leading to reputational and economic impact. I recommend that the team treat event emission more seriously in general.

Findings Information

🌟 Selected for report: 0xRajeev

Also found by: JMukesh, pauliax

Labels

bug

duplicate

G (Gas Optimization)

sponsor confirmed

addressed

Awards

0 VETH - $0.00

0.0172 ETH - $43.09

External Links

Link to GitHub issue

Handle

JMukesh

Vulnerability details

Impact

isMember(address member) function never been called in another function to check wether given address is member or not. Due to uninitialized of _isMember state variable , it will always return false which make this function useless.

Proof of Concept

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Pools.sol#L215

Tools Used

no tool used

Recommended Mitigation Steps

this function should be removed if it serve no purpose

#0 - strictly-scarce
2021-04-25T10:00:08Z

Valid

#1 - 0xBrian
2021-05-11T04:30:31Z

https://github.com/vetherasset/vaderprotocol-contracts/commit/c9c066a9c8b083a751d1f4e13850253672e10b03

#2 - dmvt
2021-05-17T17:43:13Z

duplicate of #118

Findings Information

🌟 Selected for report: 0xRajeev

Also found by: JMukesh, cmichel, pauliax, s1m0

Labels

bug

duplicate

G (Gas Optimization)

sponsor confirmed

addressed

Awards

0 VETH - $0.00

0.0084 ETH - $20.94

External Links

Link to GitHub issue

Handle

JMukesh

Vulnerability details

These are the state variable which can be declared as constant to save gas

In Vether.sol

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Vether.sol#L13

uint public override decimals = 18;

In Util.sol

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Utils.sol#L16

uint private one = 10**18;
uint private _10k = 10000;
uint private _year = 31536000;

3. In token1.sol and token2.sol

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Token2.sol#L13

uint public override decimals = 18;

In Synth.sol https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Synth.sol#L16

uint public override decimals = 18;
In Router.sol https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Router.sol#L17

uint one = 10**18;

#0 - strictly-scarce
2021-04-25T10:02:42Z

Valid

#1 - 0xBrian
2021-05-11T05:23:54Z

Most likely addressed in one of https://github.com/vetherasset/vaderprotocol-contracts/commit/66ce8d72bcd91544f5f1b0382d5e809aa6b71a44 https://github.com/vetherasset/vaderprotocol-contracts/commit/2dacdc3716706c8f635e7173b3fc5f1b42869a35

#2 - dmvt
2021-05-17T17:34:58Z

duplicate of #129

Findings Information

🌟 Selected for report: JMukesh

Also found by: 0xRajeev, a_delamo

Labels

bug

1 (Low Risk)

sponsor acknowledged

Awards

0.8799 VETH - $45.76

0.0211 ETH - $52.80

External Links

Link to GitHub issue

Handle

JMukesh

Vulnerability details

Impact

The parameter that are used in init() function to initialize the state variable,these state variable are used in other function to perform operation. since it lacks zero address validation, it will be problematic if there is error in these state variable. some of the function will loss their functionality which can cause the redeployment of contract

Proof of Concept

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Pools.sol#L43

Factory.sol

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Factory.sol#L27

Dao.sol

https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/DAO.sol#L46

Tools Used

slither

Recommended Mitigation Steps

add require condition which check zero address validation

#0 - strictly-scarce
2021-04-25T09:58:53Z

Same as: https://github.com/code-423n4/2021-04-vader-findings/issues/13#issuecomment-826294937

No issue found

Vader Protocol contest - JMukesh's results

Capital efficient liquidity protocol

General Information

Findings Distribution

Researcher Performance

External Links

[H-21] User can escape from paying fees - $1,642.54

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - Mervyn8532021-04-25T00:00:16Z

#1 - strictly-scarce2021-04-25T09:49:30Z

#2 - Mervyn8532021-05-01T08:48:29Z

#3 - dmvt2021-05-26T19:04:37Z

#4 - dmvt2021-05-26T22:25:20Z

QA Report - $511.47

Gas Report - $93.12

QA Report - $511.47

Gas Report - $93.12

[L-06] Lack of emission of event after sensitive action - $47.90

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - dmvt2021-05-26T22:35:08Z

🌟 [G-04] Public function that could be declared external - $29.09

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - strictly-scarce2021-04-25T09:58:16Z

#1 - 0xBrian2021-05-11T06:45:37Z

🚀 [L-23] events can be emitted even after failed transaction - $365.01

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - strictly-scarce2021-04-25T09:47:57Z

#1 - Mervyn8532021-05-01T08:47:45Z

#2 - dmvt2021-05-26T19:08:38Z

[G-06] isMember(address member) function never been called in another function to check wether given address is member or not - $43.09

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - strictly-scarce2021-04-25T10:00:08Z

#1 - 0xBrian2021-05-11T04:30:31Z

#2 - dmvt2021-05-17T17:43:13Z

[G-07] State variable that can be declared as constant to save gas - $20.94

#0 - Mervyn853
2021-04-25T00:00:16Z

#1 - strictly-scarce
2021-04-25T09:49:30Z

#2 - Mervyn853
2021-05-01T08:48:29Z

#3 - dmvt
2021-05-26T19:04:37Z

#4 - dmvt
2021-05-26T22:25:20Z

#0 - dmvt
2021-05-26T22:35:08Z

#0 - strictly-scarce
2021-04-25T09:58:16Z

#1 - 0xBrian
2021-05-11T06:45:37Z

#0 - strictly-scarce
2021-04-25T09:47:57Z

#1 - Mervyn853
2021-05-01T08:47:45Z

#2 - dmvt
2021-05-26T19:08:38Z

#0 - strictly-scarce
2021-04-25T10:00:08Z

#1 - 0xBrian
2021-05-11T04:30:31Z

#2 - dmvt
2021-05-17T17:43:13Z

#0 - strictly-scarce
2021-04-25T10:02:42Z

#1 - 0xBrian
2021-05-11T05:23:54Z

#2 - dmvt
2021-05-17T17:34:58Z

#0 - strictly-scarce
2021-04-25T09:58:53Z