Asymmetry contest - lopotras's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L84-L96 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L113-L119 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L182-L195 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L107-L114 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L60-L88 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L56-L67 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L156-L204 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L94-L106 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L73-L81 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L138-L155

Vulnerability details

Impact

Due to severity of the impact the description will focus on unstake as an example, as blocking it will result in inability for users to withdraw funds, whereas blocking stake will only block subsequent deposits, which creates less of a severe impact. This being said, many of the concepts described apply analogically to stake and rebalanceToWeights function.

SafEth's unstake function is designed to work in an all-or-nothing way. In case of temporary or permanent failure or pause of any one of the underlying protocols integrated through derivative contracts and used during the transaction triggered via unstake which would result in revert, funds deposited through SafEth to all integrated protocols would become locked, until all protocols are again fully functional or until an update is performed on the implementation of the Asymmetry's contracts (thanks to deployment through an upgradable proxy).

In the implementation in scope SafEth contract interacts with a total of 5 different external protocols as follows:

-- stake: Lido, Frax, Uniswap, Rocketpool -- unstake: Lido, Curve, Frax, Rocketpool -- rebalanceToWeights: all of the above

Current implementation does not consider any of the transactions to ever revert, and thus, does not provide a process to handle such an event. This means that staking through SafEth exposes 100% of user deposits to risk associated with all the underlying protocols, even though only a defined percentage of total funds are locked with any one of the protocols.

Moreover, considering that SafEth is designed to be able to integrate more than just the 3 derivatives in scope, as the product evolves the risk will only increase, in worst case scenario resulting in a temporary lock-out of all funds deposited in the protocol and potential loss of credibility.

Links to Lines of Code

-- 1-2: stake and unstake function in SafEth, shows they do not consider any called derivative contract functions to revert -- 3: addDerivatives function in SafEth, shows that system is designed to integrate more derivatives -- 4-9: withdraw and deposit functions in derivative contracts, shows that the derivative implementations do not have a process in place to handle any of the transactions revert, they will revert too -- 10: rebalanceToWeights, shows it does not consider any called derivative contract functions to revert

Proof of Concept

Consider a scenario:

-- SafEth is deployed with 3 derivatives: Reth, SfrxEth and WstEth
-- Bob deposits 1 ether to SafEth via stake
-- All liquidity is withdrawn from Lido's Curve Pool (https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L15-L16 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L61) -- Bob calls unstake, which reverts

Note that "All liquidity is withdrawn from Lido's Curve Pool" might as well be any unknown exploit performed on any of the underlying derivative protocols.

Tools Used

Manual review

Recommended Mitigation Steps

In order to limit the necessary changes to the system, each implementation of the derivative integration can handle all edge cases specific to particular integration individually, without a need to change more general logic in SafEth.

The failed withdrawals could be accounted for on each derivative as debt for each user individually. This would mean that in case of the issue occurring, an unstake transaction would succeed, burning all provided SafEth tokens, but send back only a lower amount of ETH to the caller, accounting debt for the caller on a derivative contract, allowing him/her to withdraw the funds at a later date if it becomes possible.

Attention should be put to accounting of total deposits within the system, especially in case of using rebalanceToWeights during the time the issue with the underlying protocol persists.

Below is an example of changes which could be done to mitigate the issue. As an example for mitigation Reth's withdraw function will be used: https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L107-L114

Each derivative could have two extra variables defined:

mapping(address => uint256) debts;
uint256 totalDebt;

And it's withdraw function could take an extra parameter address caller: function withdraw(uint256 amount, address caller) external;. The caller would be the msg.sender from SafEth context perspective.

An example of a modified `withdraw function is shown below:

function withdraw(uint256 amount, address caller) external onlyOwner {
        // @audit check implementation
        (bool success, ) = rethAddress().call(
            abi.encodeWithSignature("burn(uint256)", amount)
        );

        if (success) {
            (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
                ""
            );
            require(sent, "Failed to send Ether");
        } else {
            totalDebt += amount;
            debts[caller] += amount;
        }
        
    }

Instead of calling the burn function via an interface, the contract performs a low-level call to burn. If the call is successful, the function proceeds as in the original implementation.

If the call reverts, a caller's debt is accounted for individually through debts mapping as well as in totalDebt (aimed to help with global accounting);

Users could at a later time withdraw their debt directly from a derivative contract using a claimDebt function:

function claimDebt() external {
        uint256 debt = debts[msg.sender];
        require(debt != 0, "no debt");
        (bool success, ) = rethAddress().call(
            abi.encodeWithSignature("burn(uint256)", debt)
        );

        require(success, "couldn't withdraw");
        
        totalDebt -= debt;
        debt = 0;
        (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
            ""
        );
        require(sent, "Failed to send Ether");
    }

Note that this mitigation implementation assumes that all variables set from the protocol perspective (i.e. maxSlippage) are set correctly and will not result in transaction reverting.

Lines of code

Unverified input:

SafEth:

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L202-L208

Deriviatives:

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L48-L50 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L51-L53 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L58-L60

Resulting problem locations:

Deposit/Stake:

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L173-L174

Withdraw/Unstake:

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L60 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L74-L75

Vulnerability detail

Impact

maxSlippage parameter can be set to an arbitrary uint256 without any additional verification through setMaxSlippage function in SafEth. In current derivative contracts maxSlippage is subtracted from $10^{18}$ representing 100% in places linked in "Resulting problem locations": (10 ** 18 - maxSlippage)

If maxSlippage is set to a value bigger than $10^{18}$ this will revert stake and/or unstake transactions due to Solidity's arithmetic error, effectively pausing stake and/or unstake functions, depending on which derivative's maxSlippage is set over 100%.

Proof of Concept

// The following test passes
it("blocks unstaking in case of slippage > 100%", async function () {
	const derivativeCount = (await safEthProxy.derivativeCount()).toNumber();
	for (let i = 0; i < derivativeCount; i++) {
		await safEthProxy.setMaxSlippage(i, ethers.utils.parseEther("10")); // 1000% slippage
	}

	const safEthBalance = await safEthProxy.balanceOf(adminAccount.address);
	await expect(safEthProxy.unstake(safEthBalance)).to.be.reverted;
});

Problem description

setMaxSlippage is protected by onlyOwner modifier and it is expected from the owner of the SafEth contract to be acting in the best interest of the protocol. This being said, while setting the slippage by the owner anywhere within the range of 0-100% might be seen as a potentially harmful/bad decision, setting the slippage above 100%, considering the consequences described in the previous section is either a mistake or an intentional pause of the protocol.

While there is nothing implicitly wrong with pausing the protocol and the SafEth contract implements pausing functionality for both stake and unstake functions, exposing it as well through setMaxSlippage may be seen as sneaky and, in case of such mistake actually happening, potentially severely harm protocols credibility.

Recommended mitigation

Verify uint _slippage input parameter in setMaxSlippage by adding the following line (or similar) before setting the slippage:

reuire(_slippage <= 10 ** 18, "slippage too high");