H-03 MitigationConfirmed

[c4-bot-9]

Lines of code

Vulnerability details

C4 issue

H-03: V3Vault::transform does not validate the data input and allows a depositor to exploit any position approved on the transformer

Comments

When V3Vault.transform() is called, several arguments are passed into transform(). Two arguments important to us are the data and tokenId arguments. The tokenId represents the id of the token that will be manipulated. The data calldata represents arbitrary data that will be passed to a transformer contract.

The core issue here is that the data argument is not validated. data is arbitrary user data and therefore a user can pass any value they want. Here arises the problem. Not only is data arbitrary, but data also contains a copy of the tokenId. A malicious user can pass in a separate token id into data, one that is different from the tokenId argument passed into V3Vault.transform(). The transformer contract will take the modified tokenID and manipulate it even though the malicious user doesn't have permission to do so.

Mitigation

PR #29

PR #29 includes fixes for a separate issue. This mitigation discussed below will focus just on this ticket's issue.

• A Transformer contract is created. This contract contains validation logic that V3Utils can use to validate the caller and the token owner.
  • The Automator._validateOwner() function is moved to the Transformer contract. This contract will be inherited by the Transformer contracts which can use the Transformer contracts to validate token ownership and caller privilege to process a position. The following contracts now inherit from Transformer: AutoCompound, AutoRange, and LeverageTransformer.
  • Transformer now inherits the vault feature logic that whitelists which vaults are allowed to interact with the Transformer contracts.
• LeverageTransformer updates two of it's functions: leverageUp and leverageDown via adding the safety check Transformer._validateCaller(). This safety check validates that the caller:
  • If is a vault, ensures that the Vault's transformedTokenId matches the data.tokenId. Otherwise, revert.
  • If the token owner is not the msg.sender nor the LeverageTransformer contract, revert.
• AutoRange.execute() calls Transformer._validateCaller() when the msg.sender is a vault. The data.tokenId is passed into _validateCaller() confirming that the caller can manage the NFT's position. This is a reasonable check considering V3Vault.transform() may call AutoRange.execute().
• AutoCompound.execute() calls Transformer._validateCaller() when the msg.sender is a vault. The data.tokenId is passed into _validateCaller() confirming that the caller can manage the NFT's position. This is a reasonable check considering V3Vault.transform() may call AutoCompound.execute().
• V3Vault.transformedTokenId is now public. This is necessary as Transformer._validateCaller() calls V3Vault.transformedTokenId.

Based on these changes, a user is no longer able to arbitrary call V3Vault.transform() with an arbitrary data.tokenId. Each transformer contract successfully checks that the params.tokenId can be managed by the msg.sender.

Conclusion

LGTM

[c4-judge]

jhsagd76 marked the issue as satisfactory