Users can't remove liquidity while malt price is below peg defend threshold

[code423n4]

Lines of code

https://github.com/code-423n4/2023-02-malt/blob/700f9b468f9cf8c9c5cffaa1eba1b8dea40503f9/contracts/Token/Malt.sol#L120-L128

Vulnerability details

Impact

The protocol is designed to limit buys while malt price is below peg defend threshold. But it is implemented by blocking any malt token transfer which is originated from the target pool. So it unexpectedly blocks users from removing liquidity too. Considering the market conditions is likely unfavorable to users when the peg is broken, users should be allowed to remove their liquidity and sell malt.

Proof of Concept

To implement the design of limiting buys while malt price is below peg, there is a _beforeTokenTransfer() hook in Malt contract.

File: contracts\Token\Malt.sol
120:   function _beforeTokenTransfer(
121:     address from,
122:     address to,
123:     uint256 amount
124:   ) internal override {
125:     (bool success, string memory reason) = transferService
126:       .verifyTransferAndCall(from, to, amount);
127:     require(success, reason);
128:   }

It calls verifyTransferAndCall() function of TransferService contract.

File: contracts\Token\TransferService.sol
49:   function verifyTransferAndCall(
...
53:   ) external returns (bool, string memory) {
54:     (
...
59:     ) = verifyTransfer(from, to, amount);
60: 
...
76:   }

And then verifyTransfer()

File: contracts\Token\TransferService.sol
078:   function verifyTransfer(
...
082:   )
083:     public
084:     view
085:     returns (
...
090:     )
091:   {
092:     address[2] memory targets;
093:     bytes[2] memory datalist;
094: 
095:     if (verifiers[from] != address(0)) { // @audit mapping of pool address => PoolTransferVerification
096:       (
...
101:       ) = ITransferVerification(verifiers[from]).verifyTransfer(
102:           from,
103:           to,
104:           amount
105:         );
106: 
107:       if (!valid) {
108:         return (false, reason, targets, datalist);
109:       }
110:       targets[0] = target;
111:       datalist[0] = data;
112:     }
113: 
...
130:   }

Finally, verifyTransfer() function of PoolTransferVerification contract is called.

File: contracts\Token\PoolTransferVerification.sol
080:   function verifyTransfer(
081:     address from,
082:     address to,
083:     uint256 amount
084:   )
085:     external
086:     view
087:     override
088:     returns (
...
093:     )
094:   {
...
106: 
107:     if (from != address(stakeToken)) {
108:       return (true, "", address(0), "");
109:     }
110: 
...
114: 
115:     return _belowPegCheck();
116:   }

As shown on L107 and L115, if from == address(stakeToken) and _belowPegCheck() fails, then the transaction will be reverted(verifyTransfer() return false). There is no distinction between transfers triggered by liquidity removing and buys.

Tools Used

VS Code

Recommended Mitigation Steps

Add a new contract with interface similar to removeLiquidity() of UniswapHandler contract but allow users to forward removeLiquidity() call to pool. And add the new contract address to whitelist of PoolTransferVerification contract.

[Picodes]

According to the documentation: When the Malt price TWAP drops below a specified threshold (eg 2% below peg) then the protocol will revert any transaction that tries to remove Malt from the AMM pool (ie buying Malt or removing liquidity). Users wanting to remove liquidity can still do so via the UniswapHandler contract that is whitelisted in recovery mode..

Therefore it seems this is the intended behavior, especially as there is indeed a whitelist.

[c4-sponsor]

0xScotch marked the issue as sponsor disputed

[0xScotch]

As mentioned above, this is the desired behaviour. The Bonding contract also has an unbondAndBreak method that allows staked users to have their LP broken behind the scenes - which always works due to internal whitelisting.

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid