code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Note: I want to acknowledge that an automated finding on high-level centralization risk does mention the vulnerable function, addDerivative (185). I've chosen to still include this because it seems to me that simply calling out every function with the onlyOwner modifier, as we see in the automated findings, neglects to illustrate that user deposits can be entirely drained in this particular unexpected way. This vulnerability is a specific, existential threat for this protocol and should be accounted for beyond being thrown on a list with instances of low severity centralization risk. This is also relevant to consider independently since it can be mitigated without necessarily removing the onlyOwner modifier from the vulnerable function.

It’s especially important since addDerivative is one of the most likely functions to remain centralized in the long run (if the severity of this particular instance was not appropriately emphasized). All this is to say that I do not believe "centralization risk" should be treated as a catch-all for any bugs that could possibly be described as such, and the distinguishing factor here is the severity--it's not my intention to spam the contest with a pre-acknowledged finding, and I hope you'll agree that this vulnerability stands on its own despite being in the same broad category as lower risk automated findings.

Impact

Because the depositAmount of eth in stake is a uint256 returned from the external deposit functions on each derivative contract, a malicious derivative contract could simply return an astronomical number from its deposit implementation. This would subsequently mint a ton of SafEth to the attacker (since the depositAmount is then used to calculate the amount of SafEth to mint), such that they hold the vast majority of the total supply. They could then call unstake and receive the proportionate amount of eth from each derivative--i.e., all of it but dust.

This attack is limited to the owner of the SafEth contract, because only the owner can add a new derivative contract. But it still constitutes a high risk vulnerability for a number of reasons--even if best practices for wallet security are observed by the owner, and even if the owner is fully trustworthy, a single person with access to all user funds is a honeypot for all kinds of off-chain attacks. In any event, it also undermines the use case: if the protocol relies on trusting a single person, it carries none of the advantages of DeFi. And since these owner privileges are not defined explicitly, they would surely come as a surprise to users, who would deposit their funds without being made aware that such a risk exists in the code.

Proof of Concept

This attack would work with the following steps:

-- Deploy contracts -- The attacker would then wait for SafEth to gain users and deposits -- Create malicious derivative contract. This contract only needs to contain the following:

function deposit() external payable returns (uint256) {
    return 100000000000000000000; // arbitrary very large number
}

function balance() public view returns (uint256) {
    return 0; // skips malicious derivative in `unstake` loop, since there is nothing to withdraw
}

Note: in reality, a hacker would also want to implement some form of access control on their deposit function, so that other SafEth users don't come along mid-attack and unwittingly receive their own large allotments of fraudulent tokens. Alternatively, the attacker could toggle the malicious derivative contract on and off atomically on both ends of their own stake. They would also want to implement a withdrawal function to recover the eth sent to the malicious contract during their attack.

-- Pass address of malicious derivative contract to addDerivative along with any non-zero uint256 for the weight -- Call stake with any value greater than the minimum threshold defined in the SafEth contract -- Call unstake, passing the SafEth token balance minted on the previous step

unstake will iterate through each derivative contract and withdraw an amount of eth from each proportional to the attacker's share of the total supply of SafEth. Since the only upper bound on the number returned from the malicious derivative is the capacity of uint256, the attacker could mint enough SafEth to leave nothing but dust in each of the valid derivative contracts.

Tools Used

N/A

Recommended Mitigation Steps

The motivation for using the returned value for the depositAmount in stake is documented: it's meant to account for the slippage incurred in the process of depositing to the underlying derivative. This is understandable, but the risk doesn't outweigh the reward.

The simplest way to mitigate it, then, would be to calculate the depositAmount within the scope of the stake function. This could be naively approximated, if necessary, with the remainder being refunded to msg.sender. It may be possible to calculate the exact amount that will be deposited before depositing by querying each derivative contract from within SafEth, but of course, this could bloat SafEth substantially as the derivative list grows. This might lead the developers to prefer the former alternative--since both options fix the underlying issue of blindly trusting a derivative contract's deposit function, either is sufficient to fix the vulnerability.

Of course, the developers could remove the onlyOwner modifier as well, but this mitigation could be more complex, considering that it would also require larger decisions about protocol governance.

liveactionllama commented 1 year ago

Marking as invalid on behalf of the Lookout.

Reason: Out of Scope - Centralization

c4-sponsor commented 1 year ago

toshiSat marked the issue as sponsor disputed

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Overinflated severity

code-423n4 / 2023-03-asymmetry-findings