QA Report

[code423n4]

Velodrome C4 Contest Findings

Velo token race condition

• File: contracts/contracts/Velo.sol

• Summary: Possible attack on native Velo token using approve and transferFrom functions.

• Details: Velo token does not inherit from ERC20 token standard, and only possible function for allowance is approve. Using this function is known to be insecure, it is possible to transfer more tokens than it should be possible. Possible attack scenario:

  1. Alice approves Bob to transfer her N amount of tokens, calling approve(bobAddress,N).
  2. After some time Alice wants to change the allowance to different amount M. She calls approve(bobAddress, M).
  3. Bob detects Alice's call before it was mined (he finds in mempool or just know about it). He calls transferFrom(aliceAddress, bobAddress, N) with higher gas price than Alice’s second transaction.
  4. If Bob’s transaction will be executed before Alice’s, he will transfer N tokens and will be approved for M amount.
  5. Bob calls transferFrom(aliceAddress, bobAddress, M) right after Alice calls second approval.

• Github Links:

  • L36
  • L60

• Mitigation: ERC20 token should inherit from ERC20 standard functions increaseAllowance and decreaseAllowance which were designed to mitigate this unwanted behavior.

  Unchecked receiver address

• Files: contracts/contracts/Velo.sol

• Summary: Missing receiver’s address validation checks in _transfer and transferFrom functions.

• Details: If the receiver address provided in _transfer and transferFrom function is the address of the contract itself, then, it may result in irrecoverable loss of tokens and the sent tokens will get stuck in the smart contract.

• Github Links:

  • L49 in Velo.sol
  • L60 in Velo.sol

• Mitigation: creating a modifier validating that the _to address is neither 0x0 nor the smart contract's own address.

Unused Library Function

• File: contracts/libraries/Math.sol

• Details: cbrt function is present in Math.sol but is never used.

• Github Link: L22

• Mitigation: Consider commenting out cbrt function since it is not used.

Inconsistent documentation with code

• File: contracts/contracts/VotingEscrow.sol

• Summary: Comments documenting code does not match the code functionality.

• Details: Whereas comments in the dev description for _balance and balanceOf functions state Throws if _owner is the zero address., there is no assertion for _owner being non-zero address. This may create assumptions of throwing errors for zero address in other functions and eventually result in unpredicted behaviour.

• Github Links:

  • L191
  • L198

• Mitigation: Add validation logic to check non-zero address.

No emitted event in state-changing functions

• Files:

  1. contracts/contracts/Bribe.sol
  2. contracts/factories/BribeFactory.sol
  3. contracts/factories/GaugeFactory.sol
  4. contracts/factories/PairFactory.sol

• Details: State-changing functions -setGauge, addRewardToken, swapOutRewardToken deliverReward in Bribe.sol ; createBribe in BribeFactory.sol ; setTeam , createGauge and createGaugeSingle in GaugeFactory ; and setPauser , acceptPauser , setPause , setFeeManager , acceptFeeManager setFee in PairFactory.sol - do not emit requisite events.

• Github Links:

  • L30 - Bribe.sol
  • L66 - Bribe.sol
  • L76 - Bribe.sol
  • L83 - Bribe.sol
  • L9 - BribeFactory.sol
  • L17 - GaugeFactory.sol
  • L22 - GaugeFactory.sol
  • L28 - GaugeFactory.sol
  • L40 - PairFactory.sol
  • L45 - PairFactory.sol
  • L50 - PairFactory.sol
  • L55 - PairFactory.sol
  • L60 - PairFactory.sol
  • L65 - PairFactory.sol

• Mitigation: Consider creating appropriate events and emitting same in state-changing functions

  No validation logic to check zero address

• File:

  1. contracts/contracts/Bribe.sol
  2. contracts/contracts/Velo.sol

• Details:

  • Bribe.sol: addRewardToken function lacks validation logic to ensure that zero address is not added as reward token.
  • Velo.sol: Given that no validation logic exists to check zero address, it is possible to mint, send and approve to zero address.

• Github Links:

  • L66 - Bribe.sol
  • L36 - Velo.sol
  • L42 - Velo.sol
  • L49 - Velo.sol

• Mitigation: Consider writing logic to guard against zero address require(token != address(0);

[pooltypes]

Duplicate of #218

[GalloDaSballo]

 Velo token race condition

Valid NC

Unchecked receiver address

Disagree

Unused Library Function

Valid NC

Inconsistent documentation with code

Valid NC

 No emitted event in state-changing functions

Valid NC

No validation logic to check zero address

Valid Low

1 L, 4 NC