Platform: Code4rena
Start Date: 09/11/2021
Pot Size: $75,000 USDC
Total HM: 57
Participants: 27
Period: 7 days
Judge: alcueca
Total Solo HM: 49
Id: 52
League: ETH
Rank: 10/27
Findings: 5
Award: $2,689.64
🌟 Selected for report: 9
🚀 Solo Findings: 2
728.5837 USDC - $728.58
defsec
During the code review, It has been seen that createPool function does not have access control.
"https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex/pool/VaderPoolFactory.sol#L54"
none
Consider to put access control on the createPool function.
#0 - SamSteinGG
2021-11-25T12:12:50Z
This is equivalent to the pool factory of Uniswap without any access control.
#1 - alcueca
2021-12-11T06:28:38Z
I can't see an issue from removing the access control, but the sponsor might want to check for possible DoS or front-running attack vectors. Invalid.
#2 - alcueca
2021-12-11T06:43:55Z
Duplicate of #98. There you have the DoS attack.
#3 - alcueca
2021-12-11T06:45:02Z
Also setting a severity of 3, because it can effectively stop the protocol from creating new pools, and therefore function.
🌟 Selected for report: defsec
485.7225 USDC - $485.72
defsec
The consult function in the contract TwapOracle.sol fetches the asset price from a Chainlink aggregator using the latestRoundData function. However, there are no checks on timeStamp, resulting in stale prices. The oracle wrapper calls out to a chainlink oracle receiving the latestRoundData(). It then checks freshness by verifying that the answer is indeed for the last known round. The returned updatedAt timestamp is not checked.
If there is a problem with chainlink starting a new round and finding consensus on the new value for the oracle (e.g. chainlink nodes abandon the oracle, chain congestion, vulnerability/attacks on the chainlink system) consumers of this contract may continue using outdated stale data (if oracles are unable to submit no new round is started)
Navigate to "https://github.com/code-423n4/2021-11-vader/blob/607d2b9e253d59c782e921bfc2951184d3f65825/contracts/twap/TwapOracle.sol#L141" contract.
consult function does not check timestamp on the latestRoundData.
None
Consider to add checks on the return data with proper revert messages if the price is stale or the round is incomplete, for example:
(uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = ETH_CHAINLINK.latestRoundData(); require(timeStamp != 0, "...");
Consider checking the oracle responses updatedAt value after calling out to chainlinkOracle.latestRoundData() verifying that the result is within an allowed margin of freshness.
https://blog.openzeppelin.com/secure-smart-contract-guidelines-the-dangers-of-price-oracles/
#0 - SamSteinGG
2021-12-22T07:40:57Z
The TWAP oracle module has been completely removed and redesigned from scratch as LBTwap that is subject of the new audit.
🌟 Selected for report: defsec
485.7225 USDC - $485.72
defsec
On the GovernorAlpha contract, function veto has been added. Although the function behaviour is expected, duplicate veto process has not been checked on that function.
function veto(uint256 proposalId, bool support) external onlyCouncil { ProposalState _state = state(proposalId); require( _state == ProposalState.Active || _state == ProposalState.Pending, "GovernorAlpha::veto: Proposal can only be vetoed when active" ); Proposal storage proposal = proposals[proposalId]; address[] memory _targets = proposal.targets; for (uint256 i = 0; i < _targets.length; i++) { if (_targets[i] == address(this)) { revert( "GovernorAlpha::veto: council cannot veto on proposal having action with address(this) as target" ); } } VetoStatus storage _vetoStatus = proposal.vetoStatus; _vetoStatus.hasBeenVetoed = true; _vetoStatus.support = support; if (support) { queue(proposalId); } emit ProposalVetoed(proposalId, support); } /**
None
Consider to check if proposals vetoed before.
VetoStatus storage _vetoStatus = proposal.vetoStatus; require(!_vetoStatus.hasBeenVetoed, "Vetoed before");
#0 - SamSteinGG
2021-11-20T06:55:17Z
Duplicate of #61
#1 - alcueca
2021-12-10T14:53:40Z
Not a duplicate
🌟 Selected for report: defsec
161.9075 USDC - $161.91
defsec
Owner only functions that change critical parameters should emit events. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.
Missing events and timelocks do not promote transparency and if such changes immediately affect users’ perception of fairness or trustworthiness, they could exit the protocol causing a reduction in liquidity which could negatively impact protocol TVL and reputation.
There are owner functions that do not emit any events in VaderBond.sol
Missing events
https://github.com/code-423n4/2021-11-vader/blob/main/repo/vader-bond/contracts/VaderBond.sol#L128
https://github.com/code-423n4/2021-11-vader/blob/main/repo/vader-bond/contracts/VaderBond.sol#L149
See similar High-severity H03 finding OpenZeppelin’s Audit of Audius (https://blog.openzeppelin.com/audius-contracts-audit/#high) and Medium-severity M01 finding OpenZeppelin’s Audit of UMA Phase 4 (https://blog.openzeppelin.com/uma-audit-phase-4/)
None
Add events to all owner/admin functions that change critical parameters.
🌟 Selected for report: defsec
161.9075 USDC - $161.91
defsec
Vader protocol allows different tokens to be used as collateral or underlying. The Dex contracts do not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.
Vader whitelists a rebasing/deflationary/inflationary token to be used as collateral or underlying by accident. This leads to miscalculations between internal Pool accounting and the balances in the token contracts.
Code Review
Make sure token vetting accounts for any rebasing/inflation/deflation Add support in contracts for such tokens before accepting user-supplied tokens
#0 - SamSteinGG
2021-11-25T12:11:55Z
This is a similar risk present in Uniswap V2 and as such should be considered no-risk finding.
#1 - alcueca
2021-12-12T05:17:56Z
Uniswap v2 doing something is not a guarantee that there isn't a risk. They might have accepted the risk, or mitigated it off-chain.
🌟 Selected for report: defsec
defsec
Since the council parameter in the _onlyCouncil are used. In the state variable , proper check up should be done , other wise error in these state variable can lead to redeployment of contract.
Code Review
Add proper zero address validation.
🌟 Selected for report: defsec
161.9075 USDC - $161.91
defsec
The current ownership transfer process involves the current owner calling VaderReserve.transferOwnership(). This function checks the new owner is not the zero address and proceeds to write the new owner's address into the owner's state variable. If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the onlyOwner() modifier.
Manual code review
Consider implementing a two step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.
🌟 Selected for report: defsec
161.9075 USDC - $161.91
defsec
The 'DOMAIN_SEPARATOR' is not recalculated in the case of a hard fork. The variable DOMAIN_SEPARATOR in contract UniswapV2ERC20 (https://github.com/code-423n4/2021-11-vader/blob/607d2b9e253d59c782e921bfc2951184d3f65825/contracts/external/UniswapV2ERC20.sol#L26) is cached in the contract storage and will not change after being initialized. However, if a hard fork happens after the contract deployment, the domain would become invalid on one of the forked chains due to the block.chainid has changed.
A similar issue was reported in a previous contest and was assigned a severity of low: code-423n4/2021-06-realitycards-findings#166
Code Review
As an solution that you may consider applying is from Sushi Trident: https://github.com/sushiswap/trident/blob/concentrated/contracts/pool/concentrated/TridentNFT.sol#L47-L62
#0 - SamSteinGG
2021-12-16T12:20:18Z
This finding relates to a test file.
12.5116 USDC - $12.51
defsec
For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.
Example Code Location
https://github.com/code-423n4/2021-11-vader/blob/607d2b9e253d59c782e921bfc2951184d3f65825/contracts/governance/GovernorAlpha.sol#L434
Proposal storage proposal = proposals[proposalId]; uint256 eta = block.timestamp + timelock.delay();
Block.timestamp never will overflow.
Code Review
Consider applying 'unchecked' keyword where overflows/underflows are not possible.
#0 - SamSteinGG
2021-11-20T06:53:09Z
Duplicate of #43
9.0084 USDC - $9.01
defsec
Shortening revert strings to fit in 32 bytes will decrease deploy time gas and will decrease runtime gas when the revert condition has been met.
Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
Revert strings > 32 bytes are here:
Manual Review
Shorten the revert strings to fit in 32 bytes. That will affect gas optimization.
#0 - SamSteinGG
2021-11-20T06:47:00Z
Duplicate of #104
🌟 Selected for report: defsec
68.651 USDC - $68.65
defsec
From Pragma 0.8.0, ABI coder v2 is activated by default. The pragma abicoder v2 can be deleted from the repository. That will provide gas optimization.
None
ABI coder v2 is activated by default. It is recommended to delete redundant codes.
From Solidity v0.8.0 Breaking Changes https://docs.soliditylang.org/en/v0.8.0/080-breaking-changes.html
#0 - alcueca
2021-12-11T06:32:11Z
No grounds for dispute
🌟 Selected for report: defsec
68.651 USDC - $68.65
defsec
During the code review, It has been observed that the functions have not been used.
none
Delete redundant code for the gas optimization.
#0 - SamSteinGG
2021-11-25T12:11:38Z
This contract is meant to be inherited and utilized in a future iteration.
#1 - alcueca
2021-12-11T06:31:49Z
It is not stated in the documentation that the contract is meant to be inherited and used in a future iteration. The warden is not wrong.
🌟 Selected for report: ye0lde
Also found by: Meta0xNull, defsec, pants, pauliax
21.2455 USDC - $21.25
defsec
Open TODOs can hint at programming or architectural errors that still need to be fixed.
The following contract still have TO-DOs.
https://github.com/code-423n4/2021-11-vader/blob/607d2b9e253d59c782e921bfc2951184d3f65825/contracts/dex/pool/VaderPool.sol#L93 https://github.com/code-423n4/2021-11-vader/blob/607d2b9e253d59c782e921bfc2951184d3f65825/contracts/dex/pool/VaderPool.sol#L85 https://github.com/code-423n4/2021-11-vader/blob/607d2b9e253d59c782e921bfc2951184d3f65825/contracts/dex-v2/pool/VaderPoolV2.sol#L157 https://github.com/code-423n4/2021-11-vader/blob/607d2b9e253d59c782e921bfc2951184d3f65825/contracts/dex-v2/pool/VaderPoolV2.sol#L209
Manual code review.
Consider to resolve these TODOs and bubble up the errors.
#0 - SamSteinGG
2021-11-25T12:28:16Z
Duplicate of #102