Holograph contest - joestakey's results

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L416

Vulnerability details

If USDT is used for a sale at some point - either through a direct sale on the NFT collection, or sent to the collection from a marketplace sale - it will remain in the contract, as getTokenPayout(address(USDT)) calls systematically revert:

on Ethereum, USDT.transfer does not return a bool, meaning this check will always fail.

Impact

enforcer/PA1D does not have any other ERC20 withdrawal function. While enforcer/PA1D is meant to be used via delegate calls from a NFT collection contract, if the NFT contract does not have any withdrawal function either, this dust mentioned above is effectively lost.

Tools Used

Manual Analysis

Mitigation

Use OpenZeppelin's library SafeERC20.safeTransfer(): If the ERC20 token does not return any boolean upon transfer like USDT, the call still goes through. It will only revert if the ERC20.transfer call reverts or return false.

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L390

Vulnerability details

Payout recipients can call getEthPayout() to transfer the ETH balance of the contract to all payout recipients. This function makes an internal call to _payoutEth, which computes the gasCost, then proceeds to check balance - gasCost > 10000 before sending the result to the payout recipients.

The issue is that this check may fail even if the balance is sufficient: the gas cost of the call is at the expense of the caller, including the subsequent calls made to addresses[i].transfer(sending). There is no need to subtract it from the amount sent to the payout recipients.

The check can make the call to getEthPayout() revert, even though there is enough balance to pay the payout recipients.

Impact

If an authorized wallet (ie payout recipient) calls getEthPayout() with the problem described above, the call will revert.

Also note that while the price that makes this attack possible extremely low, its is not impossible if made on the sale of a single ERC1155 item, that could have an extremely low price

In summary, under these conditions:

• payoutAddresses array large enough so that balance - gasCost <= 10000
• low activity on the collection, so that address(this).balance does not increase any more.

the payout recipients cannot retrieve their payout.

The only way for the payout to be sent would be for the owner to call configurePayouts() and set a smaller number of payout recipients, in order to reduce the gasCost and make this check pass - but this does not work if the payoutAddresses array is already small.

Proof-Of-Concept

For simplicity, assume one payout address, owned by Alice:

• Alice calls getEthPayout(), which in turn calls _payoutEth()
• gasCost = (23300 * length) + length = 23300 + 1 = 23301
• balance = address(this).balance = 33000
• balance - gasCost = 33300 - 23301 = 9999 < 10000, so the check line 390 reverts
• Alice cannot retrieve her payout.

Tools Used

Manual Analysis

Mitigation

The validation should ensure balance > 10000

389:     uint256 balance = address(this).balance;
-390:     require(balance - gasCost > 10000, "PA1D: Not enough ETH to transfer");
+390:     require(balance > 10000, "PA1D: Not enough ETH to transfer");

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L391 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L395

Vulnerability details

Payout recipients can call getEthPayout() to transfer the ETH balance of the contract to all payout recipients. This function makes an internal call to _payoutEth, which sends the payment to the recipients based on their associated bp

The issue is that the balance used in the transfer calls is not the contract ETH balance, but the balance minus a gasCost.

This means getEthPayout() calls will leave dust in the contract.

Impact

If the dust is small enough, a subsequent call to getEthPayout is likely to revert because of this check. And enforcer/PA1D does not have any other ETH withdrawal function. While enforcer/PA1D is meant to be used via delegate calls from a NFT collection contract, if the NFT contract does not have any withdrawal function either, this dust mentioned above is effectively lost.

Proof-Of-Concept

Let us take the example of a payout recipient trying to retrieve their share of the balance, equal to 40_000 For simplicity, assume one payout address, owned by Alice:

• Alice calls getEthPayout(), which in turn calls _payoutEth()
• gasCost = (23300 * length) + length = 23300 + 1 = 23301
• balance = address(this).balance = 40000
• balance - gasCost = 40000 - 23301 = 16699,
• sending = ((bps[i] * balance) / 10000) = 10000 * 16699 / 10000 = 16699
• Alice receives 16699.

Alice has to wait for the balance to increase to call getEthPayout() again. But no matter what, there will always be at least a dust of 10000 left in the contract.

Tools Used

Manual Analysis

Mitigation

The transfers should be done based on address(this).balance. The gasCost is redundant as the gas amount is specified by the caller of getEthPayout(), the contract does not have to provide gas.

-391: balance = balance - gasCost;
392:     uint256 sending;
393:     // uint256 sent;
394:     for (uint256 i = 0; i < length; i++) {
395:       sending = ((bps[i] * balance) / 10000);
396:       addresses[i].transfer(sending);
397:       // sent = sent + sending;
398:     }