Ethereum Credit Guild - 0xaltego's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L216

Vulnerability details

Impact

Loss of whole staked funds

Proof of Concept

SurplusGuildMinter contract allows GUILD to be minted from CREDIT collateral. Accordingly it provides the user positions including the rewards. getRewards should be called in order to update the rewards for all the users.

The functon is below;

Contract: SurplusGuildMinter.sol

216:     function getRewards(
217:         address user,
218:         address term
219:     )
220:         public
221:         returns (
222:             uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
223:             UserStake memory userStake, // stake state after execution of getRewards()
224:             bool slashed // true if the user has been slashed
225:         )
226:     {
227:         bool updateState;
228:         lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
229: -->     if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
230:             slashed = true;
231:         }
232: 
233:         // if the user is not staking, do nothing
234:         userStake = _stakes[user][term];
235:         if (userStake.stakeTime == 0)
236:             return (lastGaugeLoss, userStake, slashed);
237: 
238:         // compute CREDIT rewards
239:         ProfitManager(profitManager).claimRewards(address(this)); // this will update profit indexes
240:         uint256 _profitIndex = ProfitManager(profitManager)
241:             .userGaugeProfitIndex(address(this), term);
242:         uint256 _userProfitIndex = uint256(userStake.profitIndex);
243: 
244:         if (_profitIndex == 0) _profitIndex = 1e18;
245:         if (_userProfitIndex == 0) _userProfitIndex = 1e18;
246: 
247:         uint256 deltaIndex = _profitIndex - _userProfitIndex;
248: 
249:         if (deltaIndex != 0) {
250:             uint256 creditReward = (uint256(userStake.guild) * deltaIndex) /
251:                 1e18;
252:             uint256 guildReward = (creditReward * rewardRatio) / 1e18;
253:             if (slashed) {
254:                 guildReward = 0;
255:             }
256: 
257:             // forward rewards to user
258:             if (guildReward != 0) {
259:                 RateLimitedMinter(rlgm).mint(user, guildReward);
260:                 emit GuildReward(block.timestamp, user, guildReward);
261:             }
262:             if (creditReward != 0) {
263:                 CreditToken(credit).transfer(user, creditReward);
264:             }
265: 
266:             // save the updated profitIndex
267:             userStake.profitIndex = SafeCastLib.safeCastTo160(_profitIndex);
268:             updateState = true;
269:         }
270: 
271:         // if a loss occurred while the user was staking, the GuildToken.applyGaugeLoss(address(this))
272:         // can be called by anyone to slash address(this) and decrement gauge weight etc.
273:         // The contribution to the surplus buffer is also forfeited.
274:         if (slashed) {
275:             emit Unstake(block.timestamp, term, uint256(userStake.credit));
276:             userStake = UserStake({
277:                 stakeTime: uint48(0),
278:                 lastGaugeLoss: uint48(0),
279:                 profitIndex: uint160(0),
280:                 credit: uint128(0),
281:                 guild: uint128(0)
282:             });
283:             updateState = true;
284:         }
285: 
286:         // store the updated stake, if needed
287:         if (updateState) {
288:             _stakes[user][term] = userStake;
289:         }
290:     }

At Line:229 the lastGaugeLoss is compared to be greater than userStake.lastGaugeLoss. If it's greater, then the function assumes that the user is slashed and it flags it at L:230

However, userStake.lastGaugeLoss was not instantiated and taken from memory instead of loading from storage. Hence, it makes the value to be zero and flag the user as slashed = true

Since getRewards is called in many places in the contract, like in unstake, if the user is slashed, they can't unstake their stakes as per this condition and early return.

Contract: SurplusGuildMinter.sol

166:         if (slashed) return;

Same applied to updateMintRatio Since the Line:298 returns the function, the rewards will not be updated as well.

Contract: SurplusGuildMinter.sol

298:         if (userStake.stakeTime == 0 || slashed) return;

Tools Used

Manual Review

Recommended Mitigation Steps

Call the Lines234-236 before 227-231

Contract: SurplusGuildMinter.sol

+         // if the user is not staking, do nothing
+         userStake = _stakes[user][term];
+         if (userStake.stakeTime == 0)
+             return (lastGaugeLoss, userStake, slashed);

227:         bool updateState;
228:         lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
229:         if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
230:             slashed = true;
231:         }
 
-           // if the user is not staking, do nothing
-           userStake = _stakes[user][term];
-           if (userStake.stakeTime == 0)
-               return (lastGaugeLoss, userStake, slashed);

Assessed type

Other

[L-01] Panic revert possibility

updateTotalRebasingShares doesn not validate currentRebasingSharePrice being less than val.targetValue which might cause panic revert.

Contract: ERC20RebaseDistributor.sol

210:         InterpolatedValue memory val = __rebasingSharePrice;
211: -->        uint256 delta = uint256(val.targetValue) - currentRebasingSharePrice;//@audit-info
212:         if (delta != 0) {

[L-02] Inability to delegate and permit

If the Guild Tokens is transferrable and planned to be used in voting, it will not be possible to use the permit and delegateBySig together due to both use _useNonce, and the nonce will change once one of them is called.

[L-03] No term validation

There's no rterm validation in stake() which might cause users not to profit anything due to the staking in non-existing terms.

Contract: SurplusGuildMinter.sol

114:     function stake(address term, uint256 amount) external whenNotPaused {
115:         // apply pending rewards
116:         (uint256 lastGaugeLoss, UserStake memory userStake, ) = getRewards(
117:             msg.sender,
118:             term
119:         );
120: 
121:         require(
122:             lastGaugeLoss != block.timestamp,
123:             "SurplusGuildMinter: loss in block"
124:         );
125:         require(amount >= MIN_STAKE, "SurplusGuildMinter: min stake");
126: 
127:         // pull CREDIT from user & transfer it to surplus buffer
128:         CreditToken(credit).transferFrom(msg.sender, address(this), amount);
129:         CreditToken(credit).approve(address(profitManager), amount);
130:  -->    ProfitManager(profitManager).donateToTermSurplusBuffer(term, amount);
131: 
132:         // self-mint GUILD tokens
133:         uint256 _mintRatio = mintRatio;
134:         uint256 guildAmount = (_mintRatio * amount) / 1e18;
135:         RateLimitedMinter(rlgm).mint(address(this), guildAmount);
136:         GuildToken(guild).incrementGauge(term, guildAmount);

[L-04] Functions expected to be called

Below is expected to be called by the users. However, due to the network being served in, it might not be profitable to call the functions due to gas costs. Advised to implemented these functions at their superior functions as inlined.

Contract: LendingTermOffboarding.sol

175:     function cleanup(address term) external whenNotPaused {

[L-05] Bribing contract / whale domination

Offboarding is fairly much more easier as per the Quorum votes. This opens the possibility of a bribing proxy contract or a whale to continously reach the Quorum and offboard the opponent term for every POLL_DURATION_BLOCKS intervals and the voting.

[N-01] Incompatibility with EIP4337

The protocol is not compatible with account abstraction due to code size checking;

Contract: ERC20MultiVotes.sol

155:     function _setContractExceedMaxDelegates(
156:         address account,
157:         bool canExceedMax
158:     ) internal {
159:         require(
160:  -->        !canExceedMax || account.code.length != 0,//@audit-info
161:             "ERC20MultiVotes: not a smart contract"
162:         ); // can only approve contracts
163: 
164:         canContractExceedMaxDelegates[account] = canExceedMax;
165: 
166:         emit CanContractExceedMaxDelegatesUpdate(account, canExceedMax);
167:     }

[N-02] The protocol is not compatible with high decimal tokens

High decimal tokens should not be accepted as gauges/terms.

Contract: SimplePSM.sol

65:     constructor(
66:         address _core,
67:         address _profitManager,
68:         address _credit,
69:         address _pegToken
70:     ) CoreRef(_core) {
71:         profitManager = _profitManager;
72:         credit = _credit;
73:         pegToken = _pegToken;
74: 
75:         uint256 decimals = uint256(ERC20(_pegToken).decimals());
76:  -->    decimalCorrection = 10 ** (18 - decimals);//@audit-info
77:     }

[N-03] Blocktime consistency

The protocol referred the block time as in 13 seconds while it's officially 12 seconds and in reality it's somewhere 12 to 13 seconds.