code-423n4 / 2023-09-centrifuge-findings

16 stars 14 forks source link

Wards cannot intervene on liquidity pools #761

Open c4-submissions opened 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/0af232255c7d045efde4ac40801dfeeed8a8d889/src/LiquidityPool.sol#L98

Vulnerability details

Impact

withApproval in LiquidityManager.sol has the documentation

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

However, the code makes it very clear that only owners have approval, never wards.

    modifier withApproval(address owner) {
        require(msg.sender == owner, "LiquidityPool/no-approval");
        _;
    }

The upshot is that only owners of positions can modify them. Wards cannot, even though the documentation makes it clear they should be able to.

Proof of Concept

I modified testDepositWithApproval in Liquidity.t.sol to try the deposit where the test contract is a ward.

        root.relyContract(lPool_, self); // give self auth permissions
        lPool.requestDeposit(amount, address(investor));

However, the transaction reverted with "LiquidityPool/no-approval", even though the caller is a ward and should have this power according to the docs.

Tools Used

Manuel inspection

Recommended Mitigation Steps

    modifier withApproval(address owner) {
        require(msg.sender == owner || wards[msg.sender] == 1, "LiquidityPool/no-approval");
        _;
    }

Assessed type

Access Control

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #41

c4-judge commented 1 year ago

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

c4-judge commented 1 year ago

gzeon-c4 marked the issue as grade-c

c4-judge commented 1 year ago

gzeon-c4 marked the issue as grade-b