withApproval is not checking if msg.sender is a ward, blocking wards to do actions in LiquidityPool.sol

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L97-L100

Vulnerability details

Impact

Contract wards are blocked from doing certain actions in LiquidityPool.sol. For example, withdraw, redeem, requestDeposit, ... This is because the withApproval modifier is not checking whether the message sender is a ward or not even though the @dev comments show that it should.

/// @dev Either msg.sender is the owner or a ward on the contract

Proof of Concept

Run the following test and verify the success

function testWardCantRequestDeposit(
    uint64 poolId,
    uint8 decimals,
    string memory tokenName,
    string memory tokenSymbol,
    bytes16 trancheId,
    uint128 price,
    uint128 currencyId,
    uint256 amount,
    uint64 validUntil
) public {
    vm.assume(currencyId > 0);
    vm.assume(amount < MAX_UINT128);
    vm.assume(amount > 1);
    vm.assume(validUntil >= block.timestamp);
    price = 2 * 10 ** 27;

    address lPool_ = deployLiquidityPool(poolId, erc20.decimals(), tokenName, tokenSymbol, trancheId, currencyId);
    LiquidityPool lPool = LiquidityPool(lPool_);

    address ward = address(0x69);
    vm.prank(address(root));
    lPool.rely(ward);

    assertEq(lPool.wards(ward), 1);

    homePools.updateTrancheTokenPrice(poolId, trancheId, currencyId, price);
    erc20.mint(self, amount);
    erc20.approve(address(investmentManager), amount);
    homePools.updateMember(poolId, trancheId, self, validUntil);

    vm.prank(ward);
    vm.expectRevert(bytes("LiquidityPool/no-approval"));
    lPool.requestDeposit(amount, self);
}

Tools Used

Manual review + vscode

Recommended Mitigation Steps

Replace require(msg.sender == owner, "LiquidityPool/no-approval"); with require(msg.sender == owner || wards[msg.sender] == 1, "LiquidityPool/no-approval");

Assessed type

Access Control

[raymondfam]

Wrong comments as confirmed by the sponsor in Discord. The code is correct.

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[c4-judge]

gzeon-c4 marked the issue as grade-c