SelfOwnedStETHBurner::recoverERC721 may allow moving ERC20s

[tinchoabbate]

Hey folks, wanted to point out what could be a minor oversight in the recoverERC721 function of the SelfOwnedStETHBurner contract. In principle it does not currently pose a risk for the system. That's why I'm opening an issue just to raise awareness.

The function currently looks like:

function recoverERC721(address _token, uint256 _tokenId) external {
    emit ERC721Recovered(msg.sender, _token, _tokenId);
    
    IERC721(_token).transferFrom(address(this), TREASURY, _tokenId);
}

While it is supposed to allow recovering ERC721 tokens, there's nothing preventing passing an arbitrary address of any contract implementing the transferFrom(address,address,uint256) interface. For example, the address of the StETH contract.

However, a griefing attack attempting to take stETH out of the burner contract in this way would fail. Because the transferFrom function of the StETH contract always requires explicit allowances. Even when the caller of transferFrom matches the source address of the transfer. That's why there does not seem to be any significant risk right now for the system.

Yet there are contracts (like DAI) that do not require allowances when the caller of transferFrom matches the source of the transfer. So if the community ever, for any reason (e.g., to follow the behavior of other ERC20 tokens), updates the StETH contract and include such functionality in its transferFrom function, as a surprising side-effect they would be unintendedly opening up the door for anyone to arbitrarily move stETH out of the burner.

I would just recommend either caution when upgrading the StETH contract, or to make things more explicit just add a check:

require(_token != LIDO, "STETH_RECOVER_WRONG_FUNC");

which is what you're doing in the recoverERC20 function already.

[TheDZhon]

Hey-hey, it's a cunning catch, thank you for the report!

More to say, the finding was not mentioned by external auditors previously, so you are the genuine discoverer here.

I guess that we will stick to the current allowance mechanics for the sake of backward compatibility, though will discuss with the team to consider the second option (i.e. explicit protection) as part of upcoming protocol upgrades.

[TheDZhon]

The issue is resolved in Lido V2 (#482)

[TheDZhon]

Decided to re-open because V2 isn't deployed on mainnet yet.

[TheDZhon]

Resolved in https://github.com/lidofinance/lido-dao/releases/tag/v2.0.0