Putty contest - auditor0517's results

Lines of code

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

Vulnerability details

Impact

Inconsistent charging admin fee for PuttyV2.exercise() and PuttyV2.withdraw(). In exercise(), it doesn't charge admin fee at all, but in withdraw() it charges the fee even when a user receives back his strike amount after order expiration.

Proof of Concept

I've contacted out.eth with this issue and he said fee must be applied if the order was exercised properly. So in exercise(), we should add a logic to take fee here. And in withdraw(), we shouldn't take a fee when an order isn't exercised here.

Tools Used

Manual Review

Recommended Mitigation Steps

• Modification for exercise()

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

• Modification for withdraw().

uint256 feeAmount = isExercised ? ((order.strike * fee) / 1000) : 0;
if (feeAmount != 0) {
    ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
}
ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount);

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L337-L339 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L359-L361 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L435-L437

Vulnerability details

Impact

If baseAsset is not WETH and a user calls PuttyV2.fillOrder() or PuttyV2.exercise() with ETH, the ETH will be locked in the contract and he can't receive it back.

Proof of Concept

This is the link to the same issue.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend checking msg.value == 0 when baseAsset is not WETH.

} else {
    require(msg.value == 0, "lost ETH");
    ERC20(order.baseAsset).safeTransferFrom(msg.sender, order.maker, order.premium);
}

Lines of code

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

Vulnerability details

Impact

A user might fail to withdraw his funds and asset. Such a scenario would happen when the strike amount is small.

Proof of Concept

Some ERC20 tokens don't allow to transfer 0 amount like this example. And in the below formula, feeAmount might be zero when order.strike is small.

feeAmount = (order.strike * fee) / 1000;

So in this case, a user can't withdraw his funds and assets forever.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend checking feeAmount is not zero before transfer.