Juicebox V2 contest - AlleyCat's results

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol#L271 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol#L315 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol#L256 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol#L301 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol#L301 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol#L384

Vulnerability details

Impact

A long list of tokens do not revert in case of failure and instead return false. If one of these tokens is used in JuiceBox, the function will not revert on transfer allowing the attacker to call these functions for free. In a cases where minReturnedTokens are returned or ERC20 tokens are transfered in exchange for these ERC20 tokens, they would be able to call them for free.

Proof of Concept

Bob calls pay function using ZRX tokens but has only dust in his wallet. ZRX token returns false instead of reverting on failure. The pay function will still return the expected amount of tokens and run the pay function as if Bob sent the correct amount of ZRX token.

ZRX ERC20 compliant code:

function transferFrom(address _from, address _to, uint _value) returns (bool) {
        if (balances[_from] >= _value && allowed[_from][msg.sender] >= _value && balances[_to] + _value >= balances[_to]) {
            balances[_to] += _value;
            balances[_from] -= _value;
            allowed[_from][msg.sender] -= _value;
            Transfer(_from, _to, _value);
            return true;
        } else { return false; }
}

Tools Used

manual review

Recommended Mitigation Steps

check the return value of your transfer/transferFrom calls for a success bool and don't rely on the failure revert.

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L809 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1199

Vulnerability details

Impact

The contract inherits from OpenZeppelin ReentrancyGuard.sol however it never uses the modifier to prevent reentry. As explained in the OZ docs

Contract module that helps prevent reentrant calls to a function.

Inheriting from ReentrancyGuard will make the nonReentrant modifier available, which can be applied to functions to make sure there are no nested (reentrant) calls to them.

Because of this a reentrancy issue is introduced at L864 when setting local state variable _fee calls _takeFromFee which updates the state variable _heldFeesOf after making external ETH transactions. Allowing an attacker to reenter the contract when calling distributePayoutsOf.

Tools

Manual Review

Recommended Mitigation Steps

Implement nonReentrant on your functions or use the checks, effects, interactions pattern to ensure there are no chances for a potential attack to reenter a contract, especially when sending ETH.