search

code-423n4 / 2022-06-putty-findings

5 stars 0 forks source link

[M-01] Fees can change during the order life-cycle #347

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L497-L503

Vulnerability details

Impact

Fee rates are not locked when fillOrder - might be unfair because admin can change contract fees unannounced. Assume an arbitrager is checking the current fees on the contract, decide it's worth and fillOrder on chain. Admin can change fees as he likes (in range 0-3%) and the new fees will be takin in account on exercise\withdraw. This unexpected and unfair behaviour might drive players and users out of the platform.

Proof of Concept

            uint256 feeAmount = 0;
            if (fee > 0) {
                feeAmount = (order.strike * fee) / 1000;
                ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
            }

            ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount);

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L497-L503

Wherever fee is taken in account, the current fee rate fee is used instead of the fee at the time of the fillOrder.

Recommended Mitigation Steps

The easiest way to do so is to save the feeAmount or the fee_rate (fee): mapping(uint256 => uint256) public feeForOrder; on the contract level feeForOrder[orderHash] = feeAmount; on the fillOrder after the feeAmount is calculated.

then using this cached value then transferring fee to owner()

outdoteth commented 2 years ago

Duplicate: Admin can change fee at any time for existing orders: https://github.com/code-423n4/2022-06-putty-findings/issues/422