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: 16/27
Findings: 3
Award: $716.28
🌟 Selected for report: 3
🚀 Solo Findings: 1
🌟 Selected for report: elprofesor
485.7225 USDC - $485.72
elprofesor
Update periods in TWAP oracles reflect risk of an asset. Updating more frequently accurately prices an asset but increases capabilities of manipulation (which should be harder with more stable assets), whereas longer update periods prevent manipulation but does not accurately price assets (due to the time difference between updates). Volatility of an asset should be considered when calculating update periods. However, in Vader's TwapOracle.sol no such considerations are made, the _updatePeriod cannot be changed after deployment of the contract. Additionally, each asset uses the same _updatePeriod which does not adequately account for the differences in risk for each asset. This could lead to price manipulation or inadequate pricing of assets.
the only chance to set _updatePeriod
_updatePeriod used in time elapsed calculation
Add the following function
function setUpdatePeriod(uint256 newUpdatePeriod) external onlyOwner { require(newUpdatePeriod > minimumUpdatePeriod, "Update period must be larger than threshold"); // Optional validation based on some risk tolerance _updatePeriod = newUpdatePeriod; }
#0 - CloudEllie
2021-11-16T15:16:49Z
Warden has requested that we add a second mitigation step/strategy to his submission:
- Consider adding a configurable update period per asset, this way we are not incorrectly assuming the same risk profile per asset.
#1 - SamSteinGG
2021-11-25T12:16:41Z
This finding is accurate as the update period should change per oracle, however, it is not of high risk severity.
#2 - alcueca
2021-12-12T05:15:18Z
Agree with sponsor, assets are not directly at risk. Severity 2.
#3 - SamSteinGG
2021-12-22T07:40:41Z
The TWAP oracle module has been completely removed and redesigned from scratch as LBTwap that is subject of the new audit.
🌟 Selected for report: elprofesor
161.9075 USDC - $161.91
elprofesor
Function veto() in Governance.sol, does not implement adequate checks to insure start block has passed. Currently votingDelay() is hard coded to 1 block which is added to the current block.number to form proposal.startBlock. When vetoing proposals adequate checks are not enforced, namely proposal.startBlock is not checked. This means that proposals are able to slip through in the same block that the veto transaction is made. Two possible attack paths (detailed in 'proof of concept') may lead to a malicious user inserting a proposal that is unexpected and having it automatically vetoed
Two possible methods of exploiting this vulnerability exist; - Accidentally mistaking the maximum proposalId - Deliberately attempting to submit a new proposal and vetoing in a single block
Of these two possibilities, the likelihood of the accidental method is higher, but both are possible attack vectors.
Assumption: There are no previously submitted proposals, therefore proposalId=0
Anyone submits proposal, call this proposal-1. The proposalId=1
Council accidentally makes a mistake in the proposalId for vetoed contracts, accidentally incrementing by 1 more than the available proposals (ie: veto(2, true))
Miner or bot detects that proposalId submitted is larger than the current proposalId in the GovernorAlpha.sol contract, reorganises the transactions and submits proposal-2. This proposal is automatically vetoed due to a lack of validation of proposal.startBlock which would normally validate the proposal has existed for at least one block.
onlyCouncil user's veto() transaction for the same block. Assuming current proposalId=3, both transactions look as such
1.1 Proposal(): call this proposal-4 (which would normally increment proposalId to 4)
1.2 submitted by council: veto(4, support): The expected proposalId after submission of the poroposal transaction would be 4 and the council may expect these to pass in the same block one after the other. Support here is whether the council is insupport or against the proposal.Prevention of front running and accidentally using a proposalId that is higher than the actual proposalId
function veto(uint256 proposalId, bool support) external onlyCouncil { ... Proposal storage proposal = proposals[proposalId]; // Check to insure proposal has passed at least one block to prevent front running. Require(block.number<>= proposal.startBlock, "VETO: Proposal pending") // @audit - IMPROVEMENT: implement start block validation to protect against frontrunning
This means if a proposal is submitted in the same block as a mistaken veto action, the veto action will revert. It is worth mentioning that this does not affect the current behaviour, where completely non-existent proposals would cause the veto action also to revert.
#0 - SamSteinGG
2021-11-25T11:52:16Z
This is not a tangible attack vector as it relies on a plethora of unlikely circumstances, including collusion of the council with a proposer which would have had wider consequences to the overall protocol.
#1 - alcueca
2021-12-11T04:41:47Z
Downgraded to severity 1 as the attack vector requires a governance takeover, and the impact would be minimal.
🌟 Selected for report: elprofesor
68.651 USDC - $68.65
elprofesor
Proposals are added to mapping after the 0th element. This leaves an null Proposal in the 0th mapping position. As a result, requires multiple checks (ie in state()) to validate whether proposal is valid.
[[ 1] GovernorAlpha, proposal(), L393] (https://github.com/code-423n4/2021-11-vader/blob/main/contracts/governance/GovernorAlpha.sol#L393) [[ 2] GovernorAlpha, state(), L280-283] (https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/governance/GovernorAlpha.sol#L280-L283)
proposals[proposalId].targets.length !=0#0 - SamSteinGG
2021-11-25T11:52:57Z
It's intended to have proposalId starting from 0 and the check ensures it.
#1 - alcueca
2021-12-11T06:58:18Z
Sponsor acknowledges the issue.