Holograph contest - seyni's results

Lines of code

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

Vulnerability details

Impact

transfer uses a fixed amount of gas, which can result in revert if the recipient is a contract. For instance when:

• The contract does not implement a payable fallback function.
• The contract implements a payable fallback function which uses more than 2300 gas units.
• The contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.

The PA1D._payoutEth function use transfer instead of call to distribute royalties. Which could lead to DoS for all royalties recipients since only one misbehaving address is needed for the entire function call to revert.

Proof of Concept

In PA1D._payoutEth royalties are distributed to the recipients addresses set by the owner in addresses using transfer. If only one of these addresses is a contract which for the any reason need more than 2300 gas to execute the transfer or doesn't implement a receive function, the call to PA1D._payoutEth will revert and no one will be able to receive his royalties.

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

            addresses[i].transfer(sending);

Blog post about this from consensys diligence (2019): https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Tools Used

Manual review.

Recommended Mitigation Steps

call should be used instead of transfer, while making sure to follow the "Checks-Effects-Interactions" pattern to prevent potential reentrancy. A reentrancy lock could also be used as additionnal safety, but would increase the gas usage.

Change this :

            addresses[i].transfer(sending);

to this :

      (bool success, ) = addresses[i].call{value: sending}("");
      require(success, "Transfer Failed");