Velodrome Finance contest - Ruhum's results

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/redeem/RedemptionReceiver.sol#L72-L105

Vulnerability details

Impact

According to the LayerZero docs, the default behavior is that when a transaction on the destination application fails, the channel between the src and dst app is blocked. Before any new transactions can be executed, the failed transaction has to be retried until it succeeds.

See https://layerzero.gitbook.io/docs/faq/messaging-properties#message-ordering & https://layerzero.gitbook.io/docs/guides/advanced/nonblockinglzapp

So an attacker is able to initiate a transaction they know will fail to block the channel between FTM and Optimism. The RedemptionSender & Receiver won't be usable anymore.

Proof of Concept

The RedemptionReceiver contract doesn't implement the non-blocking approach as seen here: https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/redeem/RedemptionReceiver.sol#L72-L105

An example implementation of the non-blocking approach by LayerZero: https://github.com/LayerZero-Labs/solidity-examples/blob/main/contracts/lzApp/NonblockingLzApp.sol

Tools Used

none

Recommended Mitigation Steps

Use the non-blocking approach as described here

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/Bribe.sol#L41-L60

Vulnerability details

Impact

An attacker can add worthless reward tokens to reach the max possible number of tokens at no cost to the Bribe contract. They deny any legit tokens from being added.

Proof of Concept

Attacker calls Bribe.notifyRewardAmount() repeatedly a worthless custom token.

  function notifyRewardAmount(address token, uint amount) external lock {
      require(amount > 0);
      if (!isReward[token]) {
        require(rewards.length < MAX_REWARD_TOKENS, "too many rewards tokens");
      }
      // bribes kick in at the start of next bribe period
      uint adjustedTstamp = getEpochStart(block.timestamp);
      uint epochRewards = tokenRewardsPerEpoch[token][adjustedTstamp];

      _safeTransferFrom(token, msg.sender, address(this), amount);
      tokenRewardsPerEpoch[token][adjustedTstamp] = epochRewards + amount;

      if (!isReward[token]) {
          isReward[token] = true;
          rewards.push(token);
          IGauge(gauge).addBribeRewardToken(token);
      }

      emit NotifyReward(msg.sender, token, adjustedTstamp, amount);
  }

After calling the function 16 times, subsequent calls will fail because of the check on L44:

      if (!isReward[token]) {
        require(rewards.length < MAX_REWARD_TOKENS, "too many rewards tokens");
      }

If the user would use a legit token, that require statement wouldn't be called.

Tools Used

none

Recommended Mitigation Steps

There are multiple ways to fix this.

1. Use a whitelist to only allow specific tokens to be used
2. Make the function permissioned
3. Use a system where you're not forced to loop over the rewards array. Then you don't have to limit the number of tokens.

Report

Low

L-01: VotingEscrow.safeTransferFrom is not ERC721 complaint

After calling IERC721Receiver(_to).onERC721Received() you have to verify that the returned value is bytes4(keccak256("onERC721Received(address,address,uint256,bytes)")).

Relevant code:

• https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/VotingEscrow.sol#L394

Non-Critical

N-01: emit an event when making changes to a contract's configuration

Whenever you make changes to a contract's config it should emit an event.

Relevant code:

./contracts/Voter.sol:82:    function setGovernor(address _governor) public {
./contracts/Voter.sol:87:    function setEmergencyCouncil(address _council) public {
./contracts/Velo.sol:26:    function setMinter(address _minter) external {
./contracts/Velo.sol:31:    function setRedemptionReceiver(address _receiver) external {
./contracts/Gauge.sol:188:    function setVoteStatus(address account, bool voted) external {
./contracts/Bribe.sol:30:  function setGauge(address _gauge) external {
./contracts/RewardsDistributor.sol:318:    function setDepositor(address _depositor) external {
./contracts/VeloGovernor.sol:39:    function setTeam(address newTeam) external {
./contracts/VeloGovernor.sol:44:    function setProposalNumerator(uint256 numerator) external {
./contracts/Minter.sol:64:    function setTeam(address _team) external {
./contracts/Minter.sol:75:    function setTeamRate(uint _teamRate) external {
./contracts/VotingEscrow.sol:1060:    function setVoter(address _voter) external {

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/main/contracts/contracts/redeem/RedemptionSender.sol#L43

Vulnerability details

Impact

When sending a msg to the layer zero endpoint you include enough gas for the transaction. If you don't include enough tokens for the gas, the transaction will fail. The RedemptionSender contract allows the user to pass any value they want which might result in them sending not enough. Their transaction will fail.

To know how much you have to send there's the estimateFees() function as described here

Proof of Concept

    function redeemWEVE(
        uint256 amount,
        address zroPaymentAddress,
        bytes memory zroTransactionParams
    ) public payable {
        require(amount != 0, "AMOUNT_ZERO");
        require(
            IERC20(weve).transferFrom(
                msg.sender,
                0x000000000000000000000000000000000000dEaD,
                amount
            ),
            "WEVE: TRANSFER_FAILED"
        );

        ILayerZeroEndpoint(endpoint).send{value: msg.value}(
            optimismChainId,
            abi.encodePacked(optimismReceiver),
            abi.encode(msg.sender, amount),
            payable(msg.sender),
            zroPaymentAddress,
            zroTransactionParams
        );
    }

Tools Used

none

Recommended Mitigation Steps

Use the estimateFees() endpoint