Livepeer contest - ye0lde's results

Handle

ye0lde

Vulnerability details

Impact

Using the existing named returns more efficiently and deleting unused or redundant code can reduce gas usage and improve code clarity.

Proof of Concept

Named return is not used in outboundTransfer: https://github.com/livepeer/arbitrum-lpt-bridge/blob/bf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2LPTGateway.sol#L70

The named part of the named return can be deleted. res is not used in the function or described in the interface for the function.

Explicit return is not needed in getOutboundCalldata: https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2LPTGateway.sol#L179

The explicit return at #179 is redundant and can be deleted.

Or you could refactor to this:

    function getOutboundCalldata(
        address token,
        address from,
        address to,
        uint256 amount,
        bytes memory data
    ) public pure returns (bytes memory ) {
       return abi.encodeWithSelector(
            IL1LPTGateway.finalizeInboundTransfer.selector,
            token,
            from,
            to,
            amount,
            abi.encode(0, data) // we don't need to track exitNums (b/c we have no fast exits) so we always use 0
        ); 
    }

Use the existing named returns more efficiently:

Change line 91 to collateralReserves = maltReserves.mul(price).div(priceTarget); https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/MaltDataLab.sol#L91

Delete the return at line #234 https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/RewardSystem/RewardDistributor.sol#L234

I suggest changing requestCapital from https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/RewardSystem/RewardOverflowPool.sol#L39-L62 to

  function requestCapital(uint256 amount)
    external
    onlyRole(REWARD_THROTTLE_ROLE, "Must have Reward throttle privs")
    returns (uint256 fulfilledAmount)  // @audit unnamed-unique
  {
    uint256 balance = auctionRewardToken.balanceOf(address(this));

    if (balance != 0) {
      // This is the max amount allowable
      fulfilledAmount = balance.mul(maxFulfillment).div(1000);

      if (amount <= fulfilledAmount) {
        fulfilledAmount = amount;
      } 

      auctionRewardToken.safeTransfer(throttler, fulfilledAmount);

      emit FulfilledRequest(fulfilledAmount);
    }
  }

I suggest changing outstandingArbTokens from https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/AuctionParticipant.sol#L104-L117 to

  function outstandingArbTokens() public view returns (uint256 outstanding) { 
    
    for (uint256 i = replenishingIndex; i < auctionIds.length; i = i + 1) {
      uint256 claimable = auction.balanceOfArbTokens(
        auctionIds[i],
        address(this)
      );

      outstanding += claimable;
    }
  }

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

See POC

Handle

ye0lde

Vulnerability details

Impact

Changing the variables from constant to immutable will reduce keccak operations and save gas.

A previous finding with additional explanation and a pointer to the ethereum/solidity issue is here: https://github.com/code-423n4/2021-10-slingshot-findings/issues/3

Proof of Concept

These variables can simply be changed from constant to immutable: https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1Migrator.sol#L114 https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1Migrator.sol#L116 https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1Migrator.sol#L121

Additional changes are needed for these variables since they are used in the constructor: https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1Migrator.sol#L111 https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2Migrator.sol#L59 https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/token/LivepeerToken.sol#L9-L10 https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/ControlledGateway.sol#L13

Here's an example of the changes needed in the constructor for: https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/ControlledGateway.sol#L13

contract ControlledGateway is AccessControl, Pausable {
    bytes32 public immutable GOVERNOR_ROLE;  

    address public immutable l1Lpt;
    address public immutable l2Lpt;

    constructor(address _l1Lpt, address _l2Lpt) {
        _setupRole(DEFAULT_ADMIN_ROLE, _msgSender());
        _setRoleAdmin(GOVERNOR_ROLE = keccak256("GOVERNOR_ROLE"), DEFAULT_ADMIN_ROLE);

        l1Lpt = _l1Lpt;
        l2Lpt = _l2Lpt;
    }

    function pause() external onlyRole(GOVERNOR_ROLE) {
        _pause();
    }

    function unpause() external onlyRole(GOVERNOR_ROLE) {
        _unpause();
    }
}

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

Change the constant variables to immutable as described in the POC.

Handle

ye0lde

Vulnerability details

Impact

Save Gas With The Unchecked Keyword (L2LPTDataCache.sol)

Redundant arithmetic underflow/overflow checks can be avoided when an underflow/overflow cannot happen.

Proof of Concept

The "unchecked" keyword can be applied here since there is an if statement before to ensure the arithmetic operations would not cause an integer underflow or overflow.: https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2LPTDataCache.sol#L57-L69

Change the code to:

    function decreaseL2SupplyFromL1(uint256 _amount) external onlyL2LPTGateway {
        // If there is a mass withdrawal from L2, _amount could exceed l2SupplyFromL1.
        // In this case, we just set l2SupplyFromL1 = 0 because there will be no more supply on L2
        // that is from L1 and the excess (_amount - l2SupplyFromL1) is inflationary LPT that was
        // never from L1 in the first place.
        unchecked {
            if (_amount > l2SupplyFromL1) {
                l2SupplyFromL1 = 0;
            } else {
                l2SupplyFromL1 -= _amount;  // @audit unchecked
            }
        }

        // No event because the L2LPTGateway events are sufficient
    }
 

A similar change can be made here: https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L2/gateway/L2LPTDataCache.sol#L91-L94

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

Add the "unchecked" keyword as shown above.

Handle

ye0lde

Vulnerability details

Impact

Variables are being assigned their default value which is unnecessary. Removing the assignment will save gas when deploying.

Proof of Concept

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1Migrator.sol#L471

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

Remove the assignments.

Or if you feel it is important to show the default assignment will occur then replace the assignments with a comment.