LoopFi - Pechenite's results

Lines of code

https://github.com/code-423n4/2024-05-loop/blob/40167e469edde09969643b6808c57e25d1b9c203/src/PrelaunchPoints.sol#L172 https://github.com/code-423n4/2024-05-loop/blob/40167e469edde09969643b6808c57e25d1b9c203/src/PrelaunchPoints.sol#L240

Vulnerability details

Impact

The PrelaunchPoints contract allows a user to lock a very small amount of LRT tokens, and right before or during a claim, the user can transfer a large amount of ETH directly to the contract.

Originally, by design, lpETH tokens should be gained only from the contract by staking LRT tokens during the locking period and then claiming them after the locking period has ended.

However, using the method described above, a user will end up avoiding uncertainty and risks associated with the staking process and being able to claim as many LRT tokens as they want even after the locking period has ended. Even though the user isn't getting those lpETH tokens for free, the user is bypassing the staking process and avoiding the risks associated with it.

Following that, the documentation also states, "Deposits are active up to the lpETH contract and lpETHVault contract are set" which is an invariant, that is broken here and further more confirms this finding.

Example scenario

The amounts are simplified for the sake of easier understanding

• Alice locks 1 LRT token and stakes it.
• Some time passes and the locking period ends.
• Before claiming, Alice sends large amount of ETH directly to the contract to gain more lpETH tokens than they have staked for during the locking period.
• Alice claims the staked amount and receives 10 lpETH tokens.
• Alice now has 10 lpETH tokens, even though they have only staked 1 LRT token during the locking period.

Proof of Concept

The following Foundry test can be added to test/PrelaunchPoints.t.sol to demonstrate this finding:

Run the test using this command: forge test --match-test "test_DepositAndStakeAfterTheClaimStartDate" -vv

function test_DepositAndStakeAfterTheClaimStartDate() public {
        uint256 lockAmount = 1;
        address user1 = vm.addr(1);

        lrt.mint(user1, lockAmount);

        vm.startPrank(user1);
        lrt.approve(address(prelaunchPoints), lockAmount);
        // Alice locks 1 LRT token
        prelaunchPoints.lock(address(lrt), lockAmount, referral);
        vm.stopPrank();

        // Set Loop Contracts and Convert to lpETH
        prelaunchPoints.setLoopAddresses(address(lpETH), address(lpETHVault));

        // Locking period ends
        vm.warp(
            prelaunchPoints.loopActivation() + prelaunchPoints.TIMELOCK() + 1
        );
        prelaunchPoints.convertAllETH();

        vm.warp(prelaunchPoints.startClaimDate() + 1);

        bytes memory data = abi.encodeWithSelector(
            0x415565b0,
            address(lrt),
            ETH,
            ((lockAmount * 1) / 100)
        );

        vm.deal(user1, 10);
        vm.prank(user1);
        // Alice sends 10 wei directly to the contract, AFTER the locking period has ended
        (bool success, ) = address(prelaunchPoints).call{value: 10}("");

        uint256 temp = lpETH.balanceOf(address(user1));
        console.log("Alice's lpETH tokens before: ", temp);

        vm.prank(user1);
        // Alice claims the staked amount and receives 10 lpETH tokens
        // Originally, Alice should have received 1 lpETH as she locked only 1 LRT token, but she received 10 lpETH tokens
        prelaunchPoints.claim(
            address(lrt),
            1,
            PrelaunchPoints.Exchange.TransformERC20,
            data
        );

        temp = lpETH.balanceOf(address(user1));
        console.log("Alice's lpETH tokens after: ", temp);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Disabling the transfer of ETH directly to the contract after the locking period or disabling the transfer of ETH directly as a whole are some possible solutions.

Assessed type

ETH-Transfer

Governance

Low

---

[G‑1] Owner can miss calling setLoopAddresses() on time

The setLoopAddresses() must be called before the loopActivation timestamp. If the owner forgets or misses to call this function on time, the lpETH and lpETHVault variables would be null and the whole contract's logic will be bricked.

    function setLoopAddresses(address _loopAddress, address _vaultAddress)
        external
        onlyAuthorized
        ❌ onlyBeforeDate(loopActivation)
    {
        lpETH = ILpETH(_loopAddress);
        lpETHVault = ILpETHVault(_vaultAddress);
        loopActivation = uint32(block.timestamp);

        emit LoopAddressesUpdated(_loopAddress, _vaultAddress);
    }

Link

[G‑2] Compromised owner can allow malicious tokens

The allowToken() function allows the owner to add any token to the allowed tokens list. If the owner's account is compromised, the attacker can add a malicious token to the allowed tokens list and break contract's logic.

    function allowToken(address _token) external onlyAuthorized {
        isTokenAllowed[_token] = true;
    }

Link

---

[L‑1] ETH sent directly for a contract will not be locked forever.

According to the NatSpec comment of the receive() function: "ETH sent to this contract directly will be locked forever." - all ethers sent directly to the contract should be locked forever. However, ETH sent directly to the contract won't be locked forever but will be distributed among users who have locked ETH.

If the contract receives ETH directly before convertAllETH() is called, the ETH in the contract will be converted to lpETH tokens and distributed among users who have locked ETH. This means that the ETH sent directly to the contract will not be locked forever.

function convertAllETH() external onlyAuthorized onlyBeforeDate(startClaimDate) {
        if (block.timestamp - loopActivation <= TIMELOCK) {
            revert LoopNotActivated();
        }

        // deposits all the ETH to lpETH contract. Receives lpETH back
        ❌ uint256 totalBalance = address(this).balance;
        lpETH.deposit{value: totalBalance}(address(this));

        // @audit modifies totalLpETH to be the balance of lpETH
        ❌ totalLpETH = lpETH.balanceOf(address(this));

        // Claims of lpETH can start immediately after conversion.
        startClaimDate = uint32(block.timestamp);

        emit Converted(totalBalance, totalLpETH);
    }

Link

and then inside the _claim() function the lpETH tokens will be distributed among users

        if (_token == ETH) {
            // @audit 'totalLpETH' will be modified and excess ETH will be distributed to users
            ❌ claimedAmount = userStake.mulDiv(totalLpETH, totalSupply);
            balances[msg.sender][_token] = 0;
            lpETH.safeTransfer(_receiver, claimedAmount);
        } else {

Link

[L‑2] Allow users to withdraw a portion of their locked tokens

Currently there is only one withdraw() function. It allows a user to withdraw all of his locked tokens, but there isn't a way to make a withdraw of specific amount of tokens.

This behavior exists in the claim functions, but is missing for withdraw which creates inconsistency and a discrepancy in the user experience.

// @audit there is no way to withdraw a specific amount of tokens
function withdraw(address _token) external {

Link

[L‑3] Implement a function for removing allowed tokens

Currently there isn't a way to remove a token from the allowed tokens list. Consider implementing a function for removing allowed tokens.

There is a function for adding allowed tokens but not for removing them.

    function allowToken(address _token) external onlyAuthorized {
        isTokenAllowed[_token] = true;
    }

Link

[L‑4] No fees are provided to the exchange proxy

According to the starter guide of 0x API, the exchange proxy contract should be provided with some wei, to cover fees..

The problem is that _fillQuote function doesn't provide any fees to the exchange proxy. Which would result in a failed swap.

        function _fillQuote(IERC20 _sellToken, uint256 _amount, bytes calldata _swapCallData) internal {
        // Track our balance of the buyToken to determine how much we've bought.
        uint256 boughtETHAmount = address(this).balance;

        require(_sellToken.approve(exchangeProxy, _amount));

        // @audit hardcoded value of 0
        ❌ (bool success,) = payable(exchangeProxy).call{value: 0}(_swapCallData);
        if (!success) {
            revert SwapCallFailed();
        }

        // Use our current buyToken balance to determine how much we've bought.
        boughtETHAmount = address(this).balance - boughtETHAmount;
        emit SwappedTokens(address(_sellToken), _amount, boughtETHAmount);
    }

Link