Ajna Protocol - sces60107's results

Lines of code

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

Vulnerability details

Impact

Before calling PositionManager.memorializePositions(), the owner of the position should call pool.increaseLPAllowance() first. Then PositionManager can receive the LPs from the owner.

However the recorded amount of received LPs is from pool.lenderInfo() which should be the full LP balance. The owner can call pool.increaseLPAllowance() with the allowance less than the full LP balance. Thus, PositionManager received LPs less than the recorded amount. And the owner can then call PositionManager.reedemPositions() to take the wrong amount of LPs.

Proof of Concept

The owner of the position can call PositionManager.memorializePositions() to transfer the LPs to PositionManager. And the amount of LP would be recorded. https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L170

    function memorializePositions(
        MemorializePositionsParams calldata params_
    ) external override {
        …

        for (uint256 i = 0; i < indexesLength; ) {
            …

            (uint256 lpBalance, uint256 depositTime) = pool.lenderInfo(index, owner);

            Position memory position = positions[params_.tokenId][index];

            // check for previous deposits
            if (position.depositTime != 0) {
                // check that bucket didn't go bankrupt after prior memorialization
                if (_bucketBankruptAfterDeposit(pool, index, position.depositTime)) {
                    // if bucket did go bankrupt, zero out the LP tracked by position manager
                    position.lps = 0;
                }
            }

            // update token position LP
            position.lps += lpBalance;
            …
        }

        // update pool LP accounting and transfer ownership of LP to PositionManager contract
        pool.transferLP(owner, address(this), params_.indexes);

        emit MemorializePosition(owner, params_.tokenId, params_.indexes);
    }

The recorded amount is from pool.lenderInfo(index, owner); which is the full LP balance.

However, in pool.transferLP(owner, address(this), params_.indexes);, it won’t transfer more than the allowance. https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/libraries/external/LPActions.sol#L244

    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 {
        …

        for (uint256 i = 0; i < indexesLength; ) {
            …

            uint256 allowedAmount = allowances_[ownerAddress_][newOwnerAddress_][index];
            if (allowedAmount == 0) revert NoAllowance();

            // transfer allowed amount or entire LP balance
            allowedAmount = Maths.min(allowedAmount, ownerLpBalance); // @audit if allowedAmount < ownerLpBalance, it only transfer allowedAmount.

            // move owner LP (if any) to the new owner
            if (allowedAmount != 0) {
                …

                owner.lps     -= allowedAmount; // remove amount of LP from old owner
                lpTransferred += allowedAmount; // add amount of LP to total LP transferred

                …
            }

            // reset allowances of transferred LP
            delete allowances_[ownerAddress_][newOwnerAddress_][index];

            unchecked { ++i; }
        }

        …
    }

Therefore, if the owner’s LP balance is 100. But the owner only set the allowance to 10. PositionManager only received 10. But 100 is recorded because of position.lps += lpBalance;

Then the owner can call Position.reedemPositions to gain 100 LPs.

    function reedemPositions(
        RedeemPositionsParams calldata params_
    ) external override mayInteract(params_.pool, params_.tokenId) {
        …

        for (uint256 i = 0; i < indexesLength; ) {
            …

            lpAmounts[i] = position.lps; // @audit PositionManager use position.lps as the amount to redeem.

            // remove LP tracked by position manager at bucket index
            delete positions[params_.tokenId][index];

            unchecked { ++i; }
        }

        address owner = ownerOf(params_.tokenId);

        // approve owner to take over the LP ownership (required for transferLP pool call)
        pool.increaseLPAllowance(owner, params_.indexes, lpAmounts);
        // update pool lps accounting and transfer ownership of lps from PositionManager contract
        pool.transferLP(address(this), owner, params_.indexes);

        emit RedeemPosition(owner, params_.tokenId, params_.indexes);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Record the actual amount of received LPs instead of lpBalance in positions[params_.tokenId][index]

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

There is an optimizer bug in PositionManager.getPositionIndexesFiltered. https://blog.soliditylang.org/2022/06/15/inline-assembly-memory-side-effects-bug/

The Yul optimizer considers all memory writes in the outermost Yul block that are never read from as unused and removes them. The bug is fixed in solidity 0.8.15. But PositionManager.sol uses solidity 0.8.14

Proof of Concept

There is an inline assembly block at the end of PositionManager.getPositionIndexesFiltered. The written memory is never read from in the same assembly block. It would trigger the bug to remove the memory write. https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L484

    function getPositionIndexesFiltered(
        uint256 tokenId_
    ) external view override returns (uint256[] memory filteredIndexes_) {
        …

        // resize array
        assembly { mstore(filteredIndexes_, filteredIndexesLength) }
    }

Unfortunately, PositionManager uses solidity 0.8.14 which would suffer from the bug. https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L3

pragma solidity 0.8.14;

Tools Used

Manual Review

Recommended Mitigation Steps

Update the solidity version to 0.8.15

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L173 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L70

Vulnerability details

Impact

In Master Spec, it says:

a. If 51% of non-treasury tokens vote affirmatively for a proposal, up to 1% of the treasury may be withdrawn by the proposal

In GRANT_FUND.md, it says:

This mechanism works by allowing up to the percentage over [50%] of non-treasury tokens, minimum threshold, that vote affirmatively to be removed from the treasury – the cap on this mechanism is therefore 100% minus the minimum threshold (50% in this case).

But the user who owns 51% of non-treasury tokens can withdraw more than 1% of treasury tokens.

Proof of Concept

In ExtraordinaryFunding._extraordinaryProposalSucceeded, it checks if votesReceived meets the requirement. https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L173

    function _extraordinaryProposalSucceeded(
        uint256 proposalId_,
        uint256 tokensRequested_
    ) internal view returns (bool) {
        uint256 votesReceived          = uint256(_extraordinaryFundingProposals[proposalId_].votesReceived);
        uint256 minThresholdPercentage = _getMinimumThresholdPercentage();

        return
            // succeeded if proposal's votes received doesn't exceed the minimum threshold required
            (votesReceived >= tokensRequested_ + _getSliceOfNonTreasury(minThresholdPercentage))
            &&
            // succeeded if tokens requested are available for claiming from the treasury
            (tokensRequested_ <= _getSliceOfTreasury(Maths.WAD - minThresholdPercentage))
        ;
    }

Suppose that the amount of non-treasury tokens is 10000 and the amount of treasury tokens is 1000. Alice owns 51% of the non-treasury token which is 5100 tokens. Alice proposes an extraordinary proposal to withdraw 100 treasury tokens. And there is no extraordinary proposal that has been executed before.

// setting
Non-treasury token’s amount = 10000
Treasury token’s amount = 1000
Alice’s token balance = 5100
tokensRequested_  = 100

// The requirement in `_extraordinaryProposalSucceeded`
(votesReceived >= tokensRequested_ + _getSliceOfNonTreasury(minThresholdPercentage)) => (votesReceived >= 100 + 10000 * 50%) => (votesReceived >= 5100)

Alice has 5100 tokens which can meet the requirement. And Alice can withdraw 100 tokens which is 10% of the treasury tokens. It breaks the spec in Master Spec

Tools Used

Manual Review

Recommended Mitigation Steps

ExtraordinaryFunding._extraordinaryProposalSucceeded needs to be modified. It should calculate the percentage of received votes. And tokensRequested_ should be bounded by the percentage over 50%.

Assessed type

Other