Platform: Code4rena
Start Date: 15/02/2022
Pot Size: $30,000 USDC
Total HM: 18
Participants: 35
Period: 3 days
Judge: GalloDaSballo
Total Solo HM: 8
Id: 87
League: ETH
Rank: 11/35
Findings: 4
Award: $930.35
🌟 Selected for report: 0
🚀 Solo Findings: 0
https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L337
when there is an emergency situation, the admin can call emergencyWithdrawERC20() to save the funds, however due to mismatch between non erc20 token like USDT and openzeppelin IERC20 where the open zeppelin IERC20 is expecting a return on the transfer function, eventhough the reality the USDT didn't return anything when doing a transfer this mismatch will make the USDT token retrieval to revert, therefore the admin can't retrieve the USDT that this contract had, and the token would be stuck.
https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L337
#0 - kphed
2022-02-18T00:39:11Z
#1 - GalloDaSballo
2022-03-06T16:23:39Z
This warden is threading really close to getting the finding invalidated as the actual impact of this finding is zero. The function is not using safeTransfer, however a function that only does the transfer won't ever fail, the worst case is the tokens not moving and being able to try again in a subsequent request.
I'll mark as Duplicate of #4
But recommend the warden to put some extra time to consider second-order consequences before submitting their findings
https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L187
the depositBribeERC20() is called from https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol, however if there is a malicious user that had depositor role, this user can call depositBribeERC20() and set the briber to the same address that the briber vault has, this will cause the token transfer to itself, In the case of WETH token when the msg.sender and the src is the same in the transferFrom function, it will treated it like a usual transfer, therefore the actual balance is still the same but the bribe amount will still inflate, and the malicious user can call this multiple times until the difference between actual balance of vault contract and the bribe amount is massive, or this user might inflate this it reach the max value of the bribe amount, which will eventually DOS this contract.
https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L187
add require(briber != address(this));
#0 - kphed
2022-02-18T00:34:15Z
Interesting thought-provoking attack though I don't think it'd cause any real harm (for the current implementation).
Duplicate of https://github.com/code-423n4/2022-02-redacted-cartel-findings/issues/1.
#1 - GalloDaSballo
2022-03-06T16:28:50Z
Duplicate of #95
63.6553 USDC - $63.66
Low : Title : proposal can be frontrun to support malicious token
Impact : When an authorized user set a proposal, a malicious user might call depositBribeERC20() right after the set proposal transaction is finish, since the token is not whitelisted, the malicious user might provide a malicious token, a token that will revert on all interaction with this contract, this can cause a dos on the proposal that just being set.
POC : https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol#L125
#0 - kphed
2022-02-18T20:44:18Z
We are going to add a token whitelist.
#1 - GalloDaSballo
2022-02-26T17:58:58Z
While there seems to be some validity to the finding, I believe that the "malicious" bribe can be sidestepped as the identifier for a bribe uses the token address
Hence multiple bribes could be set for the same proposal, using different tokens
I'd assume bribes will be claimed individually allowing to claim separately, hence the DOS is probably not possible.
Low severity may be appropriate, but nothing higher
#2 - GalloDaSballo
2022-02-27T00:23:01Z
3/10
GAS: 1. Title : Change 2**256 - 1 to type(uint).max
Impact: its cheaper to use type(uint).max instead of doing 2**256 - 1 calculation for unlimited approval
POC : https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol#L68-L69
Title : Change memory pointer to a storage pointer can save gas
Impact : In the getBribe() it uses memory pointer to retrieve the token address and bribe amount, by changing this memory pointer to a storage can save some gas +- 90 gas
POC : https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L152
Title : save length value of an array to a memory can save gas
Impact : In the claim() it is cheaper to save length value of the _claims array to a local variable first, so the check and the loop didn't have to get the length in a require check and in each loop
POC : https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/RewardDistributor.sol#L80
Title : unnecessary check on briber equal address zero
Impact : In the https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L175, this check is unnecessary because this function is being called from the https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol#L235 which the briber is set to msg.sender, since the possibility for the msg.sender to be address 0 is small, it is cheaper in gas to remove require(briber != address(0), "Invalid briber");.
POC : https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L175
#0 - drahrealm
2022-02-19T07:29:32Z
Duplicate with previous similar gas optimization tips
#1 - GalloDaSballo
2022-02-26T01:22:30Z
I don't believe number 2 to be valid
If you read from storage you're doing an SLOAD (100 on hot slot), if you read from memory you're doing and MLOAD (3 gas)
In lack of an actual proof I would not recommend the finding
#2 - GalloDaSballo
2022-02-26T01:29:43Z
3/10, would recommend adding Proof of Concept and code examples to make the findings hold when challenged