search

code-423n4 / 2023-05-maia-findings

23 stars 12 forks source link

In case a token added to ERC4626MultiToken.sol/UlyssesToken.sol gets compromised, it will not be possible to add or remove any asset #889

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-amm/UlyssesToken.sol#L60-L85

Vulnerability details

Proof of Concept

UlyssesToken.removeAsset() will make an ERC20 transfer to send the removed asset funds from UlyssesToken to the caller.

function removeAsset(address asset) external nonReentrant onlyOwner {
    ...
    asset.safeTransfer(msg.sender, asset.balanceOf(address(this)));
}

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-amm/UlyssesToken.sol#L60-L85

The issue with this approach is that if token to be removed is an ERC20Upgradeable, it would be possible to change the internal token transfer function to always revert after it was added to the vault, which would cause UlyssesToken.addAsset() and UlyssesToken.removeAsset() to always revert. UlyssesToken.removeAsset() will revert on L84 and UlyssesToken.addAsset() will revert since it will call UlyssesToken.updateAssetBalances().

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-amm/UlyssesToken.sol#L115-L119

Following scenario could happen:

  1. admin adds a token by calling UlyssesToken.addAsset(), assume the token was created using ERC20Upgradeable and the token is working normally when it's being added initially
  2. some time later, the token gets compromised - or the token creators decide to grief Maia multi token vaults - and update the token contract to always revert when calling transfer() and transferFrom()
  3. the admin tries to remove the token by calling UlyssesToken.removeAsset(), but L84 will revert
  4. since the asset cannot be removed, the admin won't be able to call UlyssesToken.addAsset() as well, because UlyssesToken.updateAssetBalances() will call the transfer function from the malicious ERC20

Note that the safe transfer variant will handle tokens that don't return a value, but it will still revert if the function being called reverts.

Impact

If a malicious ERC20 gets added into the vault, no asset will be able to be added or removed from UlyssesToken/ERC4626MultiToken, every call to UlyssesToken.addAsset() and UlyssesToken.removeAsset() will revert.

Note a malicious token cannot be added directly because the UlyssesToken.addAsset() - UlyssesToken.updateAssetBalances() would revert. Therefore, for this exploit to happen, it would have to be a ERC20Upgradeable that gets updated after it gets added to the vault.

Tools Used

Manual review.

Recommended Mitigation Steps

In UlyssesToken.removeAsset(), favor pull over push, see withdrawal from contracts pattern.

Consider removing L84 and save the value to be transferred in a mapping with the asset address as the key and the amount as the value, and create a new function to execute the transfer.

With this approach, on UlyssesToken.removeAsset() it will be possible to remove ERC20 tokens even if the transfer function for the token to be removed reverts.

Assessed type

ERC20

c4-judge commented 1 year ago

trust1995 changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

trust1995 marked the issue as grade-c