Asymmetry contest - J4de's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L63

Vulnerability details

Impact

    function stake() external payable {
// --skip--
        for (uint i = 0; i < derivativeCount; i++)
            underlyingValue +=
[1]->           (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
                    derivatives[i].balance()) /
                10 ** 18;
// --skip--
    function unstake(uint256 _safEthAmount) external {
// --skip--
        for (uint256 i = 0; i < derivativeCount; i++) {
            // withdraw a percentage of each asset based on the amount of safETH
            uint256 derivativeAmount = (derivatives[i].balance() *
                _safEthAmount) / safEthTotalSupply;
            if (derivativeAmount == 0) continue; // if derivative empty ignore
[2]->       derivatives[i].withdraw(derivativeAmount);
        }

Both the stake and unstake operations in the SafEth contract need to traverse each derivative for processing, such as code [1] and code [2]. And each derivative contract may depend on other three-party contracts.

The problem here is that there is no interface provided to delete the derivative, resulting in a problem with any derivative (although the probability is very small) may cause the entire contract to be unusable.

Proof of Concept

Tools Used

manual audit

Recommended Mitigation Steps

It is recommended to add an interface to delete derivative.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L74

Vulnerability details

Impact

    function withdraw(uint256 _amount) external onlyOwner {
// --skip--
[1]->   uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) *
            (10 ** 18 - maxSlippage)) / 10 ** 18;

        IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(
            1,
            0,
            frxEthBalance,
            minOut
        );
        // solhint-disable-next-line
        (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
            ""
        );
        require(sent, "Failed to send Ether");
    }

Code 1, first divides 10**18 and then multiplies 10**18 - maxSlippage, and this creates an unnecessary loss of precision. This may result in a decrease of up to 1 ETH in the minimum required received ETH.

Proof of Concept

Tools Used

manual audit

Recommended Mitigation Steps

It is recommended to multiply first and then divide.