Description

The VaultFacet contract has a transferReserve(_vault, _currency, _to, _amount) function meant to be used by the vault owner for recovering any ERC20 assets held by their vault.

The function calls execute on the given vault, instructing it to call transferFrom(from, to, amount) on the address specified by the _currency argument, with the to and amount arguments specified by the transferReserve caller.

An attacker can take advantage of this capability by making the vault call transferFrom on the ERC721 contract controlling a collateral held by the vault. This is possible since ERC20 transferFrom and ERC721 transferFrom signatures are identical; therefore, the calldata format required by both functions is the same.

Impact

An attacker can transfer any NFT held by a vault without having fully repaid the debt for which the NFT was held as collateral.

Recommendations

Ensure the contract being called is not the contract of an NFT being held as collateral.

An additional recommended hardening measure would be to entirely deny calls to ERC721 contracts. A possible approach to accomplish this is to try to call a harmless ERC721 method on the contract and reverting the transaction if the call does _not_ fail.

Remediation

Commit 7460dc9a↗ was indicated as containing the remediation. The commit appears to correctly fix the issue. The transferReserve function has been renamed to transferCurrency and now takes as input the address of an NFT collection. The currency to be transferred is obtained from the metadata associated to the collection in Voyage storage.

Voyage updated the code in a subsequent commit, and (as of commit f558e630↗) the transferCurrency function again receives a _currency argument representing an ERC20 contract address. The change was made to ensure users can always withdraw ERC20 tokens that would otherwise be at risk of being stuck in their vault. That address is checked against data contained in Voyage storage to ensure it is not the address of an ERC721 contract used by Voyage, and the code seems still safe.

We note that 25 commits exist between 7460dc9a↗ and the one subject to our audit, applying changes that are both irrelevant as well as others potentially relevant to the remediation, increasing the difficulty of the review. In total, the diff between the reviewed and remediation commits amounts to 18 solidity files changed, with 137 insertions and 385 deletions. The changes include adding a checkCurrencyAddr function also used in transferCurrency, then renamed to checkCollectionAddr, which only ensures that the address given as an argument is a deployed contract, not that it exists in the metadata stored by Voyage as the name could imply.