c4-bot-2 commented 4 months ago

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

Comment

In the original code, function V3Vault.transform does not validate the input data to call transformer:

(bool success,) = transformer.call(data);
        if (!success) {
            revert TransformFailed();
        }

The transformers also don't validate call data, they will proceed as long as the caller is the vault, for example AutoRange.execute:

function execute(ExecuteParams calldata params) external {
        if (!operators[msg.sender] && !vaults[msg.sender]) {
            revert Unauthorized();
        }
        ExecuteState memory state;
        PositionConfig memory config = positionConfigs[params.tokenId];
}

Attacker can provide a different tokenId in call data and transform other users positions.

Mitigation

PR #29 The mitigation code implements validation for each transformer instead of validating it in V3Vault. A new function _validateCaller is added:

function _validateCaller(INonfungiblePositionManager nonfungiblePositionManager, uint256 tokenId) internal view {
        if (vaults[msg.sender]) {
            uint256 transformedTokenId = IVault(msg.sender).transformedTokenId();
            if (tokenId != transformedTokenId) {
                revert Unauthorized();
            }
        } else {
            address owner = nonfungiblePositionManager.ownerOf(tokenId);
            if (owner != msg.sender && owner != address(this)) {
                revert Unauthorized();
            }
        }
    }

Function _validateCaller ensures that transformedTokenId the same with params.tokenId This function is then used for validations in every related transformers functions: AutoCompound.execute, AutoRange.execute, AutoRange.configToken, LeverageTransformer.leverageUp, LeverageTransformer.leverageDown,V3Utils.execute.

I think these are all functions needs this validation.

Suggestion

I think the function setOperator should also revert if the input operator is a vault:

function setOperator(address _operator, bool _active) public onlyOwner {
        emit OperatorChanged(_operator, _active);
        operators[_operator] = _active;
    }

Because if a vault is also an operator, then attacker can call V3Vault.execute with malicious data and _validateCaller is not called:

if (!operators[msg.sender]) {
            if (vaults[msg.sender]) {
                _validateCaller(nonfungiblePositionManager, params.tokenId);
            } else {
                revert Unauthorized();
            }
        }

Conclusion

LGTM

c4-judge commented 4 months ago

jhsagd76 marked the issue as satisfactory