Holograph contest - pashov's results

Lines of code

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

Vulnerability details

Proof of concept

USDT’s transfer function does not return a boolean - it is not ERC20 compliant. If the contract had a balance of USDT and getTokenPayout() or getTokenPayouts() were called to get that USDT payout, the line

require(erc20**.transfer**(addresses[i], sending), "PA1D: Couldn't transfer token");

will always revert, because USDT’s transfer doesn’t return a boolean.

Impact

The impact is forever stuck payouts in PA1D.sol essentially resulting in a loss of value.

Recommendation

Use OpenZeppelin’s SafeERC20 instead with safeTransferFrom - it works with USDT as well.

Lines of code

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

Vulnerability details

Impact

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:

1. The claimer smart contract does not implement a payable function.
2. The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.
3. The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300.

Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

Even if the current design is for using an EOA feeController you might decide to use a multisig in a later stage, and using call() will work for both, unlike transfer().

Recommendation

Use call() with value instead of transfer()