Redacted Cartel contest - IllIllI's results

Lines of code

https://github.com/code-423n4/2022-02-redacted-cartel/blob/92c4d5810df7b9de15eae55dc7641c8b36cd799d/contracts/BribeVault.sol#L330-L340

Vulnerability details

While it's important for a contract's admin to be able to fix issues, there should be limitations placed on the admin's power.

Impact

The admin role is able to withdraw any amount of tokens of any token type whenever he/she wishes (a rug pull). Even if the admin is benevolent, the fact that there is a rug vector available may negatively impact the protocol's reputation.

Proof of Concept

    function emergencyWithdrawERC20(address token, uint256 amount)
        external
        onlyRole(DEFAULT_ADMIN_ROLE)
    {
        require(token != address(0), "Invalid token");
        require(amount > 0, "Invalid amount");

        IERC20(token).transfer(msg.sender, amount);

        emit EmergencyWithdrawal(token, amount, msg.sender);
    }

https://github.com/code-423n4/2022-02-redacted-cartel/blob/92c4d5810df7b9de15eae55dc7641c8b36cd799d/contracts/BribeVault.sol#L330-L340

Tools Used

Code inspection

Recommended Mitigation Steps

Remove this function

Lines of code

https://github.com/code-423n4/2022-02-redacted-cartel/blob/92c4d5810df7b9de15eae55dc7641c8b36cd799d/contracts/BribeVault.sol#L296-L297 https://github.com/code-423n4/2022-02-redacted-cartel/blob/92c4d5810df7b9de15eae55dc7641c8b36cd799d/contracts/BribeVault.sol#L337

Vulnerability details

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made revert.

Impact

The code as currently implemented does not handle these sorts of tokens properly when they're used as bribes. The tokens can be safely added to the vault but once there, there are no methods to get them back out again which leads to a loss of funds.

Proof of Concept

Sending to the distributor and fee recipient are broken in transferBribes()

                IERC20(token).transfer(feeRecipient, feeAmount);
                IERC20(token).transfer(distributor, distributorAmount);

https://github.com/code-423n4/2022-02-redacted-cartel/blob/92c4d5810df7b9de15eae55dc7641c8b36cd799d/contracts/BribeVault.sol#L296-L297

As is the call in emergencyWithdrawERC20()

        IERC20(token).transfer(msg.sender, amount);

https://github.com/code-423n4/2022-02-redacted-cartel/blob/92c4d5810df7b9de15eae55dc7641c8b36cd799d/contracts/BribeVault.sol#L337

Tools Used

Code inspection

Recommended Mitigation Steps

Use OpenZeppelin’s SafeERC20's safeTransfer() instead

Lines of code

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L296 https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L297 https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/RewardDistributor.sol#L179

Vulnerability details

Some ERC20 tokens, such as Tether (USDT), allow for charging a fee any time transfer() or transferFrom() is called. If a contract does not allow for amounts to change after transfers, subsequent transfer operations based on the original amount will revert() due to the contract having an insufficient balance.

Impact

The current implementation does not work properly with fee-on-transfer tokens which will either cause calls to revert, or will leave later bribe recipients with fewer tokens to withdraw than expected.

Proof of Concept

The following calls are separate instances of this issue where the call may suddenly start reverting for a fee-on-transfer token when the fee is turned on right before the function call, or has bee on prior to the call:

IERC20(token).transfer(feeRecipient, feeAmount);              

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L296

IERC20(token).transfer(distributor, distributorAmount);              

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L297

IERC20(token).safeTransfer(_account, _amount);              

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/RewardDistributor.sol#L179

Tools Used

Code inspection

Recommended Mitigation Steps

One way to mitigate the issue is to measure the contract's balance right before and after the asset-transferring functions.

<array>.length should not be looked up in every loop of a for-loop

Even memory arrays incur the overhead of bit tests and bit shifts to calculate the array length 1.

for (uint256 i = 0; i < distributions.length; i++) {      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L269

for (uint256 i = 0; i < _claims.length; i++) {      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/RewardDistributor.sol#L82

for (uint256 i = 0; i < _distributions.length; i++) {      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/RewardDistributor.sol#L103

for (uint256 i = 0; i < proposals.length; i += 1) {    

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol#L152

++i/i++ should be unchecked{++i}/unchecked{++i} when it is not possible for them to overflow, as is the case when used in for- and while-loops

for (uint256 i = 0; i < distributions.length; i++) {      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L269

for (uint256 i = 0; i < _claims.length; i++) {      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/RewardDistributor.sol#L82

for (uint256 i = 0; i < _distributions.length; i++) {      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/RewardDistributor.sol#L103

for (uint256 i = 0; i < proposals.length; i += 1) {    

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol#L152

++i costs less gas than ++i, especially when it's used in for-loops (--i/i-- too)

for (uint256 i = 0; i < distributions.length; i++) {      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L269

for (uint256 i = 0; i < _claims.length; i++) {      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/RewardDistributor.sol#L82

for (uint256 i = 0; i < _distributions.length; i++) {      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/RewardDistributor.sol#L103

It costs more gas to initialize variables to zero than to let the default of zero be applied

for (uint256 i = 0; i < distributions.length; i++) {      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L269

for (uint256 i = 0; i < _claims.length; i++) {      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/RewardDistributor.sol#L82

for (uint256 i = 0; i < _distributions.length; i++) {      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/RewardDistributor.sol#L103

for (uint256 i = 0; i < proposals.length; i += 1) {    

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol#L152

require() strings longer than 32 bytes cost extra gas

require(sentDistributor, "Failed to transfer to distributor");          

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol:294:

require(amount > 0, "Bribe amount must be greater than 0");      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol:223:

require(msg.value > 0, "Bribe amount must be greater than 0");      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol:263:

Using 1e18 rather than 10**18 saves gas

return (((amount * priceOracle) / (10**18)) * (10**_ethDecimals)) /       

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol:103:

(10**_btrflyDecimals);               

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol:104:

(((amount * (10**18)) / priceOracle) *          

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol:108:

(10**_btrflyDecimals)) / (10**_ethDecimals);             

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol:109:

Functions not called by the contract itself should be external rather than public

function setRewardForwarding(address to) public {           

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol#L298

Using > 0 costs more gas than != 0 when used in a require() statement

require(ethLiquidity > 0 && btrflyLiquidity > 0, "Insufficient amounts");       

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol:140:

require(bribeIdentifier.length > 0, "Invalid bribeIdentifier");           

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol:171:

require(rewardIdentifier.length > 0, "Invalid rewardIdentifier");           

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol:172:

require(amount > 0, "Amount must be greater than 0");       

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol:174:

require(bribeIdentifier.length > 0, "Invalid bribeIdentifier");           

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol:218:

require(rewardIdentifier.length > 0, "Invalid rewardIdentifier");           

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol:219:

require(msg.value > 0, "Value must be greater than 0");       

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol:221:

require(distributions.length > 0, "Invalid distributions");           

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol:261:

require(distributorAmount > 0, "Invalid pending reward amount");         

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol:279:

require(distributions.length > 0, "Invalid distributions");           

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol:321:

require(amount > 0, "Invalid amount");           

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol:335:

require(amount > 0, "Invalid amount");           

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol:350:

require(_claims.length > 0, "Invalid _claims");           

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/RewardDistributor.sol:80:

require(_distributions.length > 0, "Invalid _distributions");           

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/RewardDistributor.sol:101:

require(proposals.length > 0, "Need at least 1 proposal");        

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol:146:

require(amount > 0, "Bribe amount must be greater than 0");      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol:223:

require(msg.value > 0, "Bribe amount must be greater than 0");      

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol:263: